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

Enabling specific preview rules #12343

Open
adrinjalali opened this issue Jul 16, 2024 · 10 comments
Open

Enabling specific preview rules #12343

adrinjalali opened this issue Jul 16, 2024 · 10 comments

Comments

@adrinjalali
Copy link
Contributor

Our usecase scikit-learn/scikit-learn#29477 is that we'd like to enable CPY001. Since it's a preview rule, one needs to enable preview mode.

However, preview mode enables many other rules which don't work as intended on our repo, give false positives, ...

That's why I looked into enabling preview rules only explicitly. However, when one does something like select = ["E", "F", "W", "I", "CPY001"] in the configuration, everything under those categories are enabled, regardless of them being in preview mode or not.

I'd argue that the above select doesn't really explicitly select those preview rules under E, F, ..., however, they're selected.

This is related to #7434, and I think explicit-preview-rules introduced in #7390 somewhat intends to fix the issue, but in this case it doesn't.

What should one do in this case?

In effect, I think I'd expect this config:

[tool.ruff.lint]
# This enables us to use CPY001: copyright header check
preview = true
# This enables us to use the explicit preview rules that we want only
explicit-preview-rules = true
# all rules can be found here: https://beta.ruff.rs/docs/rules/
select = ["E", "F", "W", "I", "CPY001"]

to only include stable rules under those categories, and enable CPY001 on top of them, and no other preview rule should be enabled.

@MichaReiser
Copy link
Member

Thanks for the detailed write up.

However, preview mode enables many other rules which don't work as intended on our repo, give false positives, ...

This is somewhat intentional because we want to encourage users to give feedback on preview rules. Whether we promote a rule to stable depends on the feedback we receive from users. Without feedback, we'll end up promoting a rule that then creates a lot of churn downstream.

This is related to #7434, and I think explicit-preview-rules introduced in #7390 somewhat intends to fix the issue, but in this case it doesn't.

Could you explain which part isn't working for you?

I tried to reproduce your setup by enabling the same rules as in your example and using explcit-preview-rules (playground). To me it is working as expected because Ruff doesn't report an error for the preview rule E201 that flags the whitespace after [ .

I can set explicit-preview-rules to false and the E201 violation then gets reported (playground)

@adrinjalali
Copy link
Contributor Author

So on scikit-learn, adding

preview = true
# This enables us to use the explicit preview rules that we want only
explicit-preview-rules = true

results in:

Found 364 errors.
No fixes available (308 hidden fixes can be enabled with the `--unsafe-fixes` option).

while on main, we get:

Found 14 errors.
$ ruff --version
ruff 0.5.1

Here are the outputs of ruff check . on the repo with and without the preview config added:

stable.txt

preview.txt

@MichaReiser
Copy link
Member

Thanks, I now see what's happening. What you're experience isn't that it enables additional preview-only rules, but that you see new violations because of some preview-only changes to stable rules.

We don't support enabling preview functionality for specific rules only. Enabling preview mode enables the preview functionality for all rules (if they have any). explicit-preview-rules only controls if preview-only rules should be enabled when using a selector like E.

@adrinjalali
Copy link
Contributor Author

Oh now I understand. However, that puts us in a weird position, because:

  • we need preview to enable CPY001
  • that enables preview sub-rules of already active rules
  • those give a lot of false positives / things we don't want to change
  • we end up outright disabling those rules
  • those rules end up not detecting legit things now since they're completely disabled

I feel like there should be a middle ground there? I hope this makes sense.

@zanieb
Copy link
Member

zanieb commented Jul 17, 2024

If those rule changes are giving you false positives in preview mode, I'd highly recommend opening issues because the behavior is likely to become stable soon.

Unfortunately we don't have enough bandwidth to manage different preview modes, that's why we have a single global flag. explicit-preview-rules is already the compromise.

An option here is to allow explicit selection of preview rules without enabling preview mode. We'd probably need to display a warning that the rule is in preview, though.

@jaraco
Copy link
Contributor

jaraco commented Jul 19, 2024

Not sure if this is the same issue when dealing with formatting.

Related to this issue, the "single flag to rule them all" has caused confusion and consternation. My projects adopted the setting in order to make "quote-style=preserve" work for compatibility when migrating from black, but that caused "hugged parenthesis" to be adopted implicitly. Later, when quote-style=preserve graduated to stable, we tried to remove the setting but that would roll back other behaviors.

In my projects, I'm opting to have "preview=true" by default (and the uncertainty that will bring), but at least that will allow the projects to continue to shift left (work at head).

I realize it's difficult to manage feature-specific flags, but just be aware that the user experience is muddled by the shifting meaning of this flag over time and releases.

@ChrisLoveringSendient
Copy link

I came across this as we're having very similar issues. I want to enable RUF029 which is currently in preview, but don't want to enable preview rules for the prefixes I have in my select.

I came across explicit-preview-rules which says

When enabled, preview rules will not be selected by prefixes — the full code of each preview rule will be required to enable the rule.

As stated above this doesn't seem to be working as documented, since I have the F prefix in my select, but after enabling preview & explicit preview rules, F841 starts reporting violations, even though the full code isn't in my select

@MichaReiser
Copy link
Member

As stated above this doesn't seem to be working as documented, since I have the F prefix in my select, but after enabling preview & explicit preview rules, F841 starts reporting violations, even though the full code isn't in my select

The option only changes how rule selectors work. F841 isn't a preview rule; that's why it gets selected when using F.

What you are looking for is a way to opt-in to preview behavior on a per-rule basis, which isn't supported.

@ChrisLoveringSendient
Copy link

ChrisLoveringSendient commented Jan 15, 2025

Oh, so it is. I might have stumbled across another oddity then.

I have this config which doesn't raise any issues when running ruff check . Other than the warning RUF029 has no effect because preview is not enabled.

extend-select = ["I", "UP", "ASYNC105", "ASYNC220", "ASYNC222", "ASYNC230", "ASYNC251", "RUF029"]

However, adding this then shows all 8 F841 failures in my repo, and then also shows RUF029 failures, but no other preview rules.

preview = true
explicit-preview-rules = true

Doesn't this suggest that explicit-preview-rules is now working as documented in that link (and as OP requested) where only RUF029 is being enabled as preview?

The unknown here is why F841 wasn't running before, but is after enabling preview?

@MichaReiser
Copy link
Member

Doesn't this suggest that explicit-preview-rules is now working as documented in that link (and as OP requested) where only RUF029 is being enabled as preview?

The author faces the same problem as you described. They see new errors for stable rules because they opted into the preview behavior.

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

No branches or pull requests

5 participants