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

--extend-select and --extend-ignore work in unexpected ways #2300

Closed
not-my-profile opened this issue Jan 28, 2023 · 15 comments · Fixed by #2312
Closed

--extend-select and --extend-ignore work in unexpected ways #2300

not-my-profile opened this issue Jan 28, 2023 · 15 comments · Fixed by #2312
Labels
question Asking for support or clarification

Comments

@not-my-profile
Copy link
Contributor

$ ruff --select E501 --ignore ALL scripts
scripts/add_rule.py:44:89: E501 Line too long (92 > 88 characters)
$ ruff --select E501 --extend-ignore ALL scripts
# (nothing)

I would expect --extend-select and --extend-ignore to simply append the values to the --select and --ignore values however they in fact take precedence over the former.

I find the current behavior to be unintuitive, is it intentional?

@charliermarsh
Copy link
Member

Yes it's intentional.

@charliermarsh
Copy link
Member

@charliermarsh
Copy link
Member

Closing as this is correct from my perspective but happy to keep discussing.

@not-my-profile
Copy link
Contributor Author

If it's intentional I think the options should rather be called --override-* because that would be more accurate than extend ... my point is that it doesn't extend.

@charliermarsh
Copy link
Member

I'm open to it. Though I'd like it to be done in a backwards-compatible way, and I'd prefer not to support all of --select, --extend-select (with old semantics), and --override-select (with the existing semantics), so I'd likely want to just not support --extend-select.

@charliermarsh
Copy link
Member

Although, --override-select suggests to me that it's clearing out the select and replacing it entirely -- which isn't the case. The extends just get applied last.

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jan 28, 2023

To be honest I don't find your reasoning in #1245 convincing.

If I understand correctly you say that it's unintuitive that --extend-select F401 doesn't do anything when you have ignore = ["F401"] in your pyproject.toml ... however we could simply have CLI options take precedence over pyproject.toml ... that doesn't require extend-select to always be applied last.

That is I'd argue even --select F401 should work when you have ignore = ["F401"] in your pyproject.toml.

@charliermarsh
Copy link
Member

There are a lot of cases to consider.

The primary motivation for this change wasn't the CLI. It was that, if you don't use these semantics for extend-select and extend-ignore in a pyproject.toml, it becomes impossible for configuration files that extend other configuration files to turn rules on and off in common cases.

Specifically, prior to that change, if you had a pyproject.toml like this:

[tool.ruff]
select = ["F"]
ignore = ["F401"]

And then a child that extended it:

[tool.ruff]
extend = "../pyproject.toml"
extend-select = ["F401"]

F401 would not be turned on. And in fact, it'd be impossible for the child to turn it on.

This change allowed us to treat the resolution in two passes: resolve the (select, ignore) rules, then apply the (extend-select, extend-ignore) rules.

Happy to consider a proposal that takes all of these cases into account. What you describe sounds like replacing --extend-select and --extend-ignore on the CLI with --select and --ignore, but perhaps it's slightly different.

@charliermarsh charliermarsh added the question Asking for support or clarification label Jan 28, 2023
@charliermarsh charliermarsh reopened this Jan 28, 2023
@charliermarsh
Copy link
Member

however we could simply have CLI options take precedence over pyproject.toml

What does "take precedence" here mean precisely? Could you provide examples of the expected behavior? In a sense, it does take precedence right now, since it overrides the select.

Is the suggestion that if a user provides either --select or --ignore, we omit any selection and ignore rules specified in pyproject.toml entirely? Just trying to understand the exact semantics of what you're proposing.

@charliermarsh
Copy link
Member

To be clear: I'd love to make this stuff more intuitive. I won't personally be working on it right now, but I'm very open to suggestions. I just want to make sure we're thinking through all the implications and that I'm clearly explaining why certain things work the way they do right now.

@not-my-profile
Copy link
Contributor Author

Specifically, prior to that change, if you had a pyproject.toml like this [..] And then a child that extended it [..] F401 would not be turned on. And in fact, it'd be impossible for the child to turn it on.

Right I agree that this is a problem however I think the obvious cause is that Configuration::combine is combining select, ignore, extend_select and extend_ignore all independently, whereas it should preserve them together.

@charliermarsh
Copy link
Member

Like, keep every foursome intact, then when resolving, resolve the first foursome, then pass those rules along and "apply" the next foursome, etc.?

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jan 28, 2023

Yes. This would also mean that latter selects override earlier ignores which currently isn't the case but I think really should be.

@charliermarsh
Copy link
Member

Would we have any need for extend-select and extend-ignore in that world?

@charliermarsh
Copy link
Member

I feel like you need a PhD in --select to understand all the possible resolution semantics. Makes me very sad!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Asking for support or clarification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants