Also recognize lintr ignore markers

[lorenzwalthert]

@MichaelChirico what do you think about this? From NEWS.md:

multiple stylerignore patterns can be specified at once and lintr markers
# nolint, # nolint start and # nolint end are now by default recognized
in addition to # styler: off and # styler: on (#849).

Closes #848

[codecov-commenter]

Codecov Report

Merging #849 (f447c1e) into main (88595fc) will decrease coverage by 0.00%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #849      +/-   ##
==========================================
- Coverage   90.04%   90.04%   -0.01%     
==========================================
  Files          47       47              
  Lines        2480     2490      +10     
==========================================
+ Hits         2233     2242       +9     
- Misses        247      248       +1     

Impacted Files	Coverage Δ
R/nest.R	100.00% <ø> (ø)
R/zzz.R	0.00% <0.00%> (ø)
R/stylerignore.R	100.00% <100.00%> (ø)
R/detect-alignment.R	97.29% <0.00%> (-1.24%)	⬇️

---

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 88595fc...f447c1e. Read the comment docs.

[github-actions]

This is how benchmark results would change (along with a 95% confidence interval in relative change) if 27b8ab9 is merged into master:

•   :ballot_box_with_check:cache_applying: 27ms -> 26.8ms [-1.42%, +0.16%]
•   :ballot_box_with_check:cache_recording: 1.11s -> 1.11s [-0.26%, +0.45%]
•   :ballot_box_with_check:without_cache: 2.91s -> 2.9s [-0.8%, +0.09%]

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

[MichaelChirico]

I'm not sure about turning it on by default, though the extension to recognize multiple is good for allowing them to be set in sync if so desired.

indeed I just wrote some code this week like

# nolint start: line_length_linter. styler: off.
...
# nolint end: line_length_linter. styler: on.

which would look cleaner if there's a single symbol.

[lorenzwalthert]

stylerignore markers must be alone on a line, so you code won't work.

styler::style_text('
# nolint start styler: off
call+1
# nolint end styler: on
')
#> 
#> # nolint start styler: off
#> call + 1
#> # nolint end styler: on

^{Created on 2021-10-09 by the reprex package (v2.0.1)}

Yet another alternative would be to make this work too by matching the regex styler: off in the comment instead of an exact match of the comment. I think we should support multiple markers anyways as you say. So, should we in addition:

• do the regex match instead of exact match
• or default to lintr and stylerignore markers (as this PR proposes).

?

I honestly think the number of cases when you want styler but not lintr or vice versa is very limited, so I think people should opt out in that case.

[MichaelChirico]

Probably the most common styler: off case that I see/use myself is for allowing whitespace alignment:

cbind(
  c( 'a',  'b',  'c',  'd'),
  c(1.11, 1.23, 1.44, 1.61)
)

And many similar cases where we want to use whitespace to make the code cleaner to read. In those cases I still want (most) lints to apply... so I'm not sure what the default should be.

Is there any empirical check we can do? How reliable do we think CRAN revdeps are for giving us insight here?

Definitely think a regex match is good... or as another suggestion, an exact match of styler: off on the text of a COMMENT node in the AST (prevents matching inside code, like "# styler: off")

[lorenzwalthert]

Probably the most common styler: off case that I see/use myself is for allowing whitespace alignment:

Then probably we should extend alignment detection :D

Is there any empirical check we can do? How reliable do we think CRAN revdeps are for giving us insight here?

I don't think that will help, as I don't think a lot of packages have explicit {styler} tests.

Definitely think a regex match is good... or as another suggestion, an exact match of styler: off on the text of a COMMENT node in the AST (prevents matching inside code, like "# styler: off")

I would only match text within a COMMENT for sure to avoid the case you describe.

[lorenzwalthert]

I just opted for regex matching of a single marker, keeping the default as is. Please see NEWS.md @MichaelChirico and let me know what you think.

[github-actions]

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

•   :ballot_box_with_check:cache_applying: 34.9ms -> 34.8ms [-3.88%, +3.12%]
•   :ballot_box_with_check:cache_recording: 1.58s -> 1.6s [-0.01%, +2.34%]
•   :ballot_box_with_check:without_cache: 4.17s -> 4.17s [-1.78%, +1.46%]

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

[lorenzwalthert]

@MichaelChirico any final thoughts?