Outdent code extracted from indented chunks

[MichaelChirico]

Closes #1945

This handles the issue at hand, not sure if @AshesITR would rather avoid further entrenching ourselves in the current setup :)

[codecov-commenter]

Codecov Report

Merging #1949 (d4bf0b5) into main (8df42ec) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d4bf0b5 differs from pull request most recent head 2098652. Consider uploading reports for the commit 2098652 to get more accurate results

@@           Coverage Diff           @@
##             main    #1949   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files         113      113           
  Lines        4972     4980    +8     
=======================================
+ Hits         4920     4928    +8     
  Misses         52       52           

Impacted Files	Coverage Δ
R/extract.R	98.68% <100.00%> (+0.15%)	⬆️

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

[MichaelChirico]

(ignoring the lint that's turned up & covered by #1879)

[AshesITR]

Will this make the lints show in the correct position?
I'm mostly worried about the column_number field.

[MichaelChirico]

Will this make the lints show in the correct position?
I'm mostly worried about the column_number field.

Good point. I guess it will be wrong... print(lints) will look fine, but plugin tools will highlight the wrong area (IINM).

Is that enough not to merge here? It seems to me like this is an improvement & we could just mark a TODO for now, WDYT?

[AshesITR]

Currently we don't have this problem and wrong lint positions may be very confusing (f.ex. if a whitespace lint lands on the wrong whitespace).
I don't really use indented code blocks though, so I'm not really qualified to judge which bug is worse.

@klmr what do you prefer to have in lintr 3.1.0, false positives for indentation or wrong lint locations for indented code blocks, if they contain lints?

[klmr]

Tough question! Personally I'd prefer if correct code didn't trigger any false positives, since linter warnings are actually fairly noisy in most editors. As a consequence this leads users (well, at least me personally) to disable linting entirely rather than having even just a handful of false positives to deal with. I'd actually love to hear how other users think about this.

[MichaelChirico]

that was my thinking too. bad lint metadata isn't an issue on lint-free code. and indented code blocks should be relatively rare anyway.

OTOH we should throw a warning in the meantime when this is detected

[AshesITR]

Is there an easy way to detect this?
Or can we even fix it by simply matching the lints line metadata with the actual file lines as a postprocessing step in lint()?

[MichaelChirico]

Is there an easy way to detect this?

maybe I'm not getting the question, but in get_source_expression, yes, it's just indents>0. at lint time, no, that metadata is not passed.

I'll take another look at how much refactoring would be needed to fix this more properly.

there's a related bug about keeping chunk-level metadata around -- that would be a decent solution here (since the indentation is a chunk-level data point).

worst case, yes, I think we can inefficiently reconstruct "good" column numbers by matching the lint text to the contents (except maybe in the case of all-blank lines)

[AshesITR]

Ah okay fine. The disadvantage of signalling in get_source_expressions() is that such a warning would pop up even for code without lints.

[AshesITR]

This note seems kind of hard to understand.
Maybe

get_source_expressions() correctly extracts indented code chunks from R Markdown documents, which helps avoid spurious lints related to whitespace (#1945, @MichaelChirico).

WDYT?

[MichaelChirico]

Great! Also added a note about using the leftmost column in the weird staggered case.