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

Error when linting fails #1227

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Error when linting fails #1227

wants to merge 6 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented May 25, 2022

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 #1226

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

@hadley hadley changed the title Lint error Error when linting fails May 25, 2022
@AshesITR
Copy link
Collaborator

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.

@hadley
Copy link
Member Author

hadley commented May 25, 2022

@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
Copy link
Member Author

hadley commented May 25, 2022

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

@AshesITR
Copy link
Collaborator

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?

@MichaelChirico
Copy link
Collaborator

MichaelChirico commented Nov 9, 2023

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?

@hadley
Copy link
Member Author

hadley commented Nov 10, 2023

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.

@AshesITR
Copy link
Collaborator

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

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.

Should lint_package() error on CI?
4 participants