Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix (hopefully) all roxygen parsing problems for dont* sequences and escape characters #729

Merged
merged 29 commits into from
Feb 15, 2021

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Feb 5, 2021

Maybe there are other keywords in https://developer.r-project.org/parseRd.pdf we have to fix later. Closes #728. The solution approach is also outlined there. Leverage roxygen2 infrastructure to generate Rd like output, then use tools::parse_Rd() (which we used wrongly on demasked roxygen comments, which created escaping problems). Also closes #731 (some speed implications, but not reflected in the benchmark).

We also considered adding more examples to the {touchstone} benchmark since there was only one roxygen2 code styling example (and it was correctly formatted) but then figured that in {styler}, 10% of all function declarations had roxygen code examples, which would translate to ~1 example in the benchmark package. The benchmark does not include a \dontrun{} (which does not make it faster than not containing any, as needs_rd_emulation() is ran anyways) but not another escape sequence (on which it would most likely be slower).

Final benchmarks:

  • cache_applying: 0.02 -> 0.02 (5.8%)
  • cache_recording: 0.76 -> 0.75 (-0.3%)
  • without_cache: 2.36 -> 2.49 (5.8%)

@lorenzwalthert lorenzwalthert changed the title Fix (hopefully) ALL roxygen parsing problems for dont* sequences and escape characters Fix (hopefully) all roxygen parsing problems for dont* sequences and escape characters Feb 5, 2021
@lorenzwalthert
Copy link
Collaborator Author

lorenzwalthert commented Feb 6, 2021

Uggg, speed benchmarks:

  • cache_applying: 0.03 -> 0.03 (4.9%)
  • cache_recording: 0.97 -> 1.17 (20.3%)
  • without_cache: 3 -> 3.28 (9.4%)

Let's remove the expensive roxygen parsing when there are no characters to escape.

bcca577

  • cache_applying: 0.03 -> 0.03 (3.8%)
  • cache_recording: 1.19 -> 1.2 (0.4%)
  • without_cache: 3.66 -> 3.83 (4.7%)

tighter regex 9e9c8b9

  • cache_applying: 0.03 -> 0.03 (3.6%)
  • cache_recording: 0.89 -> 0.9 (1.6%)
  • without_cache: 2.67 -> 2.87 (7.4%)

remove fix for #731 (4c5366b)

  • cache_applying: 0.03 -> 0.03 (4.2%)
  • cache_recording: 1.05 -> 1.07 (2.1%)
  • without_cache: 3.27 -> 3.34 (1.9%)

@codecov-io
Copy link

Codecov Report

Merging #729 (72b40e4) into master (3560b39) will decrease coverage by 0.06%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #729      +/-   ##
==========================================
- Coverage   90.57%   90.50%   -0.07%     
==========================================
  Files          47       47              
  Lines        2334     2381      +47     
==========================================
+ Hits         2114     2155      +41     
- Misses        220      226       +6     
Impacted Files Coverage Δ
R/transform-files.R 97.01% <20.00%> (-2.99%) ⬇️
R/roxygen-examples-parse.R 96.22% <95.74%> (-3.78%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3560b39...72b40e4. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

braces in roxygen comments cause styling error avoid turning \n into \\n in roxygen docs
2 participants