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

Improve warnings when ignoring preview rules without preview flag #7491

Closed
bersbersbers opened this issue Sep 18, 2023 · 11 comments · Fixed by #7842
Closed

Improve warnings when ignoring preview rules without preview flag #7491

bersbersbers opened this issue Sep 18, 2023 · 11 comments · Fixed by #7842
Assignees
Labels
preview Related to preview mode features

Comments

@bersbersbers
Copy link
Contributor

ruff check --isolated --ignore PLC1901,PLR0904  # version 0.0.290

outputs

warning: Selection of nursery rule `PLC1901` without the `--preview` flag is deprecated.
warning: Selection `PLR0904` has no effect because the `--preview` flag was not included.

I think these warnings are rather irrelevant - they basically confirm what the users wants to do anyway, which is not select these rules.

@charliermarsh charliermarsh added cli Related to the command-line interface needs-decision Awaiting a decision from a maintainer preview Related to preview mode features and removed cli Related to the command-line interface labels Sep 18, 2023
@silverwind
Copy link

silverwind commented Sep 21, 2023

Also noticed this strange warning. I had PLC1901 in my ignore config for a long time (because it's a unsafe rule), ruff v0.0.290 now suddenly started warning about it.

@hmvp
Copy link

hmvp commented Sep 21, 2023

Also noticed this strange warning. I had PLC1901 in my ignore config for a long time (because it's a unsafe rule), ruff v0.0.290 now suddenly started warning about it.

That rule moved to the nursery, probably because its unsafe... We had the same..

@silverwind
Copy link

Does nursery mean it's disabled by default now when I select=["PL"]?

@charliermarsh
Copy link
Member

@silverwind - Yeah, if you do select=["PL"], PLC1901 will not be included.

@silverwind
Copy link

Thanks, I'll remove it then from ignore and I see the warning also makes sense. It should probably explain better what the "nursery" is, the online docs also have no info on this topic.

@charliermarsh
Copy link
Member

"Nursery" was changed to "Preview" -- we should probably update the docs to reflect that it used to be called "Nursery".

@silverwind
Copy link

I see. Maybe just reword the warning to something like this?

warning: Ignoring preview rule `<id>` has no effect unless `--preview` is specified.

@charliermarsh
Copy link
Member

\cc @zanieb - when you're back :)

@zanieb zanieb changed the title Remove unnecessary warnings when ignoring preview rules without preview flag Improve warnings when ignoring preview rules without preview flag Sep 26, 2023
@zanieb
Copy link
Member

zanieb commented Sep 26, 2023

"select" is kind of a broad term in this warning, we're referring to the use of a rule selector which can be used with any of the command line options e.g. --select / --ignore. I figured this would be a little ambiguous and we could improve it with more careful messaging in a follow-up — thanks for your feedback!

@zanieb zanieb removed the needs-decision Awaiting a decision from a maintainer label Sep 26, 2023
zanieb added a commit that referenced this issue Sep 27, 2023
@frenck
Copy link
Contributor

frenck commented Oct 2, 2023

Just ran into this on the Home Assistant project. Honestly, I don't think this is a messaging problem.

A rule was ignored explicitly and still gives us this warning. Meaning if you run it with --preview, it still was ignored. But that is now something we should not do anymore?

It is a bit odd as we don't want it in preview but also want to ignore it if it gets out of preview/nursery.

I'm not sure why an ignore flag has to trigger this warning. What is the goal for triggering the warning in that case? What is the risk if it would still be set to be ignored while only available in preview?

I'm just confused about what problem is solved here and why this warning is added for this specific case reported in this issue.

../Frenck

@zanieb
Copy link
Member

zanieb commented Oct 2, 2023

Hey @frenck

The implementation is just naively collecting all of the selector options e.g.

for selector in selection
.select
.iter()
.chain(selection.fixable.iter())
.flatten()
.chain(selection.ignore.iter())
.chain(selection.extend_select.iter())
.chain(selection.unfixable.iter())
.chain(selection.extend_fixable.iter())
{

This warning is not explicitly targetting use of --ignore <RULE>, it applies to any option in which <RULE> is used. This was just the simplest way to implement the warning alongside our existing warnings, but we can update that iterator to be aware of the specific option used to remove or reword the message as needed.

I'll be working on this soon.

zanieb added a commit that referenced this issue Oct 8, 2023
…7842)

Closes #7491

Users found it confusing that warnings were displayed when ignoring a
preview rule (which has no effect without `--preview`). While we could
retain the warning with different messaging, I've opted to remove it for
now. With this pull request, we will only warn on `--select` and
`--extend-select` but not `--fixable`, `--unfixable`, `--ignore`, or
`--extend-fixable`.
konstin pushed a commit that referenced this issue Oct 11, 2023
…7842)

Closes #7491

Users found it confusing that warnings were displayed when ignoring a
preview rule (which has no effect without `--preview`). While we could
retain the warning with different messaging, I've opted to remove it for
now. With this pull request, we will only warn on `--select` and
`--extend-select` but not `--fixable`, `--unfixable`, `--ignore`, or
`--extend-fixable`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preview Related to preview mode features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants