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

Support ruff rule rule-name #14666

Open
InSyncWithFoo opened this issue Nov 29, 2024 · 8 comments
Open

Support ruff rule rule-name #14666

InSyncWithFoo opened this issue Nov 29, 2024 · 8 comments
Labels
cli Related to the command-line interface

Comments

@InSyncWithFoo
Copy link
Contributor

This is how a rule's documentation is rendered in PyCharm (with the help of RyeCharm):

D203 is one of the rare cases (~10) where a rule's documentation has a link to another rule's. Currently, clicking on such a link from the documentation popup would open the browser. As a plugin developer, I want to avoid this and instead resolve the link in place, same as what I did with option links (e.g., lint.extend).

However, I have no way to know what rule a name refers to; if I knew its code, ruff rule <code> would work, but documentation links use hyphenated names, and ruff rule <name> has yet to be implemented.

This issue is similar to #14348 and is a part of #1773.

@dhruvmanila dhruvmanila added the cli Related to the command-line interface label Nov 29, 2024
@MichaReiser
Copy link
Member

This is great UX, and I like the idea of intercepting the rule links so that they appear in the editor instead.

I've some UX concerns about mixing rule codes and rule names in the CLI. It will set a precedence that other commands should accept rule names too.

Have you considered querying all rules once running ruff rule --all --output-format=json. The output contains the name for every rule (and its documentation). I don't expect it to be much more expensive than querying the documentation for a single rule.

@MichaReiser
Copy link
Member

Another consideration here is that this PR technically makes renaming rules a breaking change.

@InSyncWithFoo
Copy link
Contributor Author

@MichaReiser I did consider --all, but the output is over 1 MB, compared to just ~1KB of a single rule. As projects can have different Ruff executables, 1 MB of JSON is a bit too large to be kept as a cache, though I will have to consider that if the PR is rejected.

Querying, parsing and rendering repeatedly seems like a waste, considering all other tasks the IDE must be doing at the same time. Data for dependency version inlay hints, for example, are retrieved once every 5 seconds (configurable) to ensure real-time feedback:

The most important point, however, is that querying by name feels more human.

We can require --preview for this change and stabilize it sometimes later; in fact, I considered doing so from the start, but rule didn't already have --preview.

@MichaReiser
Copy link
Member

I did consider --all, but the output is over 1 MB, compared to just ~1KB of a single rule. As projects can have different Ruff executables, 1 MB of JSON is a bit too large to be kept as a cache, though I will have to consider that if the PR is rejected.

This doesn't seem too bad considering an IDE context, and you can reduce it to a simple name -> code mapping, which should be very compact.

@sbrugman
Copy link
Contributor

It will set a precedence that other commands should accept rule names too.

Isn't this exactly the direction we would like to be headed?

Charlie already gave it some thought and said he was in favour:

We should definitely do it.

#1773 (comment)

As noted in this comment there is already some precedence of using human-names in the docs.

This change is a great next step towards tackling #1773 (human-friendly names) and #1774 (categories) and/or #3363 (presets).

Of course we need to be consider such as retaining numeric codes for backwards compatibility, ensuring the quality of names and mechanics such as accepting rule codes and rule names interchangeably.

@MichaReiser Could the Astral team internally discuss that given these considerations we have green light to accept rule names in general, or that a detailed proposal is required? This seems to me like the most effective way of moving forward.

@MichaReiser
Copy link
Member

I agree; we definitely want #1773.

I don't think this change brings us significantly closer to #1773. It doesn't tackle the hard problems but now introduces inconsistency in the CLI interface.

Could the Astral team internally discuss that given these considerations we have green light to accept rule names in general, or that a detailed proposal is required? This seems to me like the most effective way of moving forward.

I understand that our current sentiment is not to support rule names until #1774 is figured out, or at least requires a detailed proposal on how it interacts and a commitment to tackle it as a whole.

@sbrugman
Copy link
Contributor

Ok clear. It wasn't apparent to me that using the human-friendly names had a dependency on the recategorisation/presets, and if it wouldn't have then consistently supporting codes and names throughout the CLI would make sense (at least to me).

Then let's conclude that a detailed proposal taking into account the complexities of #1773, #1774 and #3363 is needed to move the needle!

@InSyncWithFoo
Copy link
Contributor Author

[...] you can reduce it to a simple name -> code mapping, which should be very compact.

Cache invalidation is most certainly not my cup of tea, but if there really is no other way...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants