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

stylerignore sequences must always be in one block #1082

Merged
merged 8 commits into from
Dec 26, 2022
Merged

Conversation

lorenzwalthert
Copy link
Collaborator

@lorenzwalthert lorenzwalthert commented Dec 26, 2022

With this PR, styling is correct for a stylerignore sequence that contains a cached expression. The solution involves ensuring that a stylerignore sequence is always processed in one block. Closes #1072.

full <- c(
  '# styler: off',
  'skip_if_not_installed("tibble")',
  'local({',
  'x<-1',
  '})',
  '# styler: on'
)
without_ignore <- full[c(-1, -length(full))]
styler:::local_test_setup(cache = TRUE)
styler:::style_text(without_ignore)
#> skip_if_not_installed("tibble")
#> local({
#>   x <- 1
#> })
styler:::style_text(full)
#> # styler: off
#> skip_if_not_installed("tibble")
#> local({
#> x<-1
#> })
#> # styler: on

Created on 2022-12-26 with reprex v2.0.2

@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2022

Codecov Report

Merging #1082 (5b8061a) into main (509bea1) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1082      +/-   ##
==========================================
+ Coverage   91.05%   91.10%   +0.04%     
==========================================
  Files          46       46              
  Lines        2684     2698      +14     
==========================================
+ Hits         2444     2458      +14     
  Misses        240      240              
Impacted Files Coverage Δ
R/expr-is.R 86.11% <ø> (ø)
R/rules-line-breaks.R 100.00% <ø> (ø)
R/style-guides.R 99.43% <ø> (ø)
R/nest.R 100.00% <100.00%> (ø)
R/stylerignore.R 100.00% <100.00%> (ø)
R/transform-block.R 100.00% <100.00%> (ø)
R/transform-files.R 97.48% <100.00%> (+0.06%) ⬆️
R/roxygen-examples.R 98.43% <0.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 509bea1 is merged into main:

  •   :ballot_box_with_check:cache_applying: 49.2ms -> 50.3ms [-1.39%, +5.83%]
  •   :ballot_box_with_check:cache_recording: 849ms -> 850ms [-1.58%, +1.93%]
  •   :ballot_box_with_check:without_cache: 1.97s -> 1.98s [-1.5%, +2.16%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert lorenzwalthert changed the title stylerignore sequences must always in one block stylerignore sequences must always be in one block Dec 26, 2022
@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 509bea1 is merged into main:

  •   :ballot_box_with_check:cache_applying: 48.4ms -> 48.6ms [-0.33%, +1.02%]
  •   :ballot_box_with_check:cache_recording: 853ms -> 853ms [-0.82%, +0.87%]
  •   :ballot_box_with_check:without_cache: 2.01s -> 2.02s [-0.6%, +0.67%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@lorenzwalthert
Copy link
Collaborator Author

@IndrajeetPatil maybe you can have a look if you have time. I know the internals are quite complex, so I don't expect you to understand that. Maybe more just if the docs are grammatically understandable and maybe if we should add more tests and if yes which ones.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 509bea1 is merged into main:

  •   :ballot_box_with_check:cache_applying: 44ms -> 44.2ms [-1.13%, +2.15%]
  •   :ballot_box_with_check:cache_recording: 756ms -> 761ms [-0.08%, +1.3%]
  •   :ballot_box_with_check:without_cache: 1.77s -> 1.77s [-0.35%, +0.56%]

Further explanation regarding interpretation and methodology can be found in the documentation.

Copy link
Collaborator

@IndrajeetPatil IndrajeetPatil left a comment

Choose a reason for hiding this comment

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

Thanks, @lorenzwalthert! Looks like this must have been a nightmare to debug.

I couldn't follow all the changes in the code, but the comments and the tests confirm that this fixes the original issue.

I think the current set of tests is enough for now. I already styled a few repos to check for any regressions, and found none.

@github-actions
Copy link
Contributor

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 99f6502 is merged into main:

  •   :ballot_box_with_check:cache_applying: 43.8ms -> 44ms [-0.07%, +1.37%]
  •   :ballot_box_with_check:cache_recording: 761ms -> 763ms [-0.42%, +0.93%]
  •   :ballot_box_with_check:without_cache: 1.81s -> 1.81s [-0.55%, +1.04%]

Further explanation regarding interpretation and methodology can be found in the documentation.

@IndrajeetPatil IndrajeetPatil merged commit 93914e4 into main Dec 26, 2022
@IndrajeetPatil IndrajeetPatil deleted the issue-1072-2 branch December 26, 2022 16:31
@lorenzwalthert
Copy link
Collaborator Author

Great, thanks @IndrajeetPatil 🥳. To me, this was a major bug and I wonder why no one before found it. Glad we solved it.

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.

Combination of stylerignore directives and local() leads to problematic styling
3 participants