Skip to content

Avoid eval(parse(...)) internally#7156

Merged
MichaelChirico merged 10 commits intomasterfrom
onload-options-call
Jul 15, 2025
Merged

Avoid eval(parse(...)) internally#7156
MichaelChirico merged 10 commits intomasterfrom
onload-options-call

Conversation

@MichaelChirico
Copy link
Copy Markdown
Member

@MichaelChirico MichaelChirico commented Jul 10, 2025

eval(parse()) in a loop calling options() many time is replaced by a single call to options() without eval(parse()).

Also tuck in a style & a tiny performance improvement to .checkTypos().

@MichaelChirico MichaelChirico changed the title Simplify setting options in .onLoad Avoid eval(parse(...)) internally Jul 10, 2025
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 10, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.77%. Comparing base (a080833) to head (80f788f).
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7156   +/-   ##
=======================================
  Coverage   98.77%   98.77%           
=======================================
  Files          81       81           
  Lines       15185    15187    +2     
=======================================
+ Hits        14999    15001    +2     
  Misses        186      186           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jul 10, 2025

  • HEAD=onload-options-call stopped early for DT[by,verbose=TRUE] improved in #6296
  • HEAD=onload-options-call slower P<0.001 for memrecycle regression fixed in #5463
    Comparison Plot

Generated via commit 80f788f

Download link for the artifact containing the test results: ↓ atime-results.zip

Task Duration
R setup and installing dependencies 2 minutes and 49 seconds
Installing different package versions 40 seconds
Running and plotting the test cases 2 minutes and 39 seconds

@MichaelChirico
Copy link
Copy Markdown
Member Author

This also fixes a 10-year-old mistake in fd37731 -- c(a = 1, b=NULL) drops b, so that option has never been set in .onLoad(), where the intention was to set it to NULL.

@MichaelChirico MichaelChirico requested a review from aitap July 14, 2025 21:31
Copy link
Copy Markdown
Member

@aitap aitap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! A fine way to rewrite the function. I was going to suggest regmatches, but this is simple enough.

@MichaelChirico MichaelChirico merged commit ae5b4a5 into master Jul 15, 2025
12 checks passed
@MichaelChirico MichaelChirico deleted the onload-options-call branch July 15, 2025 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants