lintr icon indicating copy to clipboard operation
lintr copied to clipboard

Error when linting fails

Open hadley opened this issue 2 years ago • 4 comments

Two ideas here:

  • Instead of quiting R, error — this will have the same impact when running R non-interactively (e.g. in GHA), but is less intrusive when working interactively.
  • Default to erroring when linting "fails"

Fixes #1126

If this seems like a worthwhile approach I'll update the docs, news, etc.

hadley avatar May 25 '22 16:05 hadley

Hmm I like the idea of stopping with an error in non-interactive mode, but for interactive mode, I don't see the benefit over what print.lints() has to offer - e.g. opening the source markers pane with a list of lints.

Also, before making this breaking change, we should probably check what on-the-fly linting plugins do and expect in terms of output.

AshesITR avatar May 25 '22 17:05 AshesITR

@AshesITR this doesn't change the user experience of the existing interactive method. The source markers pane still opens, and the only difference is that when you change back to the console, you'll see more clearly that linting was not successful.

I suspect this will have relatively limited downstream implications because most lintr usage is interactive. It may lead to more GitHub lintr workflows failing, but I suspect most folks would be surprised that they weren't failing before.

hadley avatar May 25 '22 19:05 hadley

Ooooh I mis-read the very long if block; let me put the error in the right place.

hadley avatar May 25 '22 19:05 hadley

I'm more worried about what RLanguageServer & co. do when R signals an error. Not using these plugins so I can't test unfortunately.

@renkun-ken IIRC you were using vscode-r, right? Do you have an idea what RLanguageServer would do if lintr starts stop()ing if it finds a lint?

AshesITR avatar May 25 '22 20:05 AshesITR

Agree that q() is the wrong behavior, but have my doubts about the changed default.

It's not uncommon for me to do something like

lint_dir(dir, linter())

# flood of lints

# oh, darn, I forgot to assign that
lints <- .Last.value

# analyze lints with as.data.frame() etc

My main concern is erroring by default makes this workflow more onerous.

At first I thought it would make this workflow impossible, but .Last.value does still capture the right output of lint_dir(), i.e. a "lints"-classed object. But it will keep throwing an error every time we try and look at it.


Is there another example you can think of where a print() method throws an error by default?

MichaelChirico avatar Nov 09 '23 05:11 MichaelChirico

It'd feel weird for print() to error, but maybe that's the least worst solution?

Another option is to have a specific last_lint() helper.

hadley avatar Nov 10 '23 15:11 hadley

last_lints() seems like a nice addition regardless of what we do here.

AshesITR avatar Nov 10 '23 17:11 AshesITR