Allow overriding pydocstyle convention rules#8586
Allow overriding pydocstyle convention rules#8586charliermarsh merged 8 commits intoastral-sh:mainfrom
Conversation
|
df3c320 to
1db79e4
Compare
|
This looks good to me, thanks for putting it together! I'd suggest adding a test or two to |
|
(I think the implementation is fine. We already special-case conventions here, so I don't mind tracking a bit of extra state for it.) |
| carryover_ignores = Some(&selection.ignore); | ||
| } | ||
|
|
||
| docstring_overrides = docstring_override_updates; |
There was a problem hiding this comment.
I think it could be fine to omit docstring_override_updates and just add to docstring_overrides whenever we see a direct selector. In other words, I don't know if this "reset" behavior is important for the purpose of these overrides. I'm trying to think of a case where this would matter.
As an example of why this exists (to the best of my memory): imagine you have select=D and ignore=D401 in your configuration file. If you then pass --select D4, we want to ignore the ignore. So we treat a --select as resetting the selection chain.
Eh, thinking about it further, what you have here is probably right. Imagine you have select=D415 in your configuration file, and then you run with --select=D. We probably still want to enforce the configuration override for D415 in that case?
There was a problem hiding this comment.
Hm... I was actually thinking about this in terms of having multiple ruff.tomls (e.g. in a monorepo or something). In that case, I think the intended behavior is that select "clears" the state of the "parent" ruff.tomls, and personally I think it'd be confusing if the docstring conventions were the only counter-example to that.
|
Alright! Added some unit tests and an integration test, so this should be ready for review! |
CodSpeed Performance ReportMerging #8586 will not alter performanceComparing Summary
|
| success: false | ||
| exit_code: 1 | ||
| ----- stdout ----- | ||
| -:2:5: D417 Missing argument description in the docstring for `log`: `base` |
There was a problem hiding this comment.
FWIW, D417 was the rule I was surprised was disabled in the numpy convention. It looks like there's specific code written to handle some of the quirks (e.g.
), so it seemed odd that it was disabled.
crates/ruff_workspace/src/options.rs
Outdated
| /// As conventions force-disable all rules not included in the convention, | ||
| /// enabling _additional_ rules on top of a convention is currently | ||
| /// unsupported. | ||
| /// If you must, you can enable rules not included in a convention by |
There was a problem hiding this comment.
Is there another place I should update the docs, or is this the right place?
There was a problem hiding this comment.
This is the right place :)
ae8eb2b to
7cba23f
Compare
| "#; | ||
|
|
||
| // If we only select the prefix, then everything passes | ||
| assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) |
There was a problem hiding this comment.
FWIW, I found some of this to be a bit repetitive -- would you be open to a PR that tries to refactor some of this Command construction into a custom builder struct?
There was a problem hiding this comment.
Sure, I'm open to the idea!
charliermarsh
left a comment
There was a problem hiding this comment.
This is great -- thanks for taking it on!
|
Appreciate all the tests, thanks for following up there. |
Summary
This fixes #2606 by moving where we apply the convention ignores -- instead of applying that at the very end, e track, we now track which rules have been specifically enabled (via
Specificity::Rule). If they have, then we do not apply the docstring overrides at the end.Test Plan
Added unit tests to
ruff_workspaceand an integration test toruff_cli