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

[readme.md] Automated link to pylint documentation using pre-commit #2546

Conversation

Pierre-Sassoulas
Copy link
Contributor

This permit to have a link to the pylint documentation explaining the message using a pre-commit hook. Result here: https://github.com/Pierre-Sassoulas/ruff/tree/automated-link-to-pylint-doc#pylint-pl

README.md Outdated Show resolved Hide resolved
@JonathanPlasse
Copy link
Contributor

Not everyone uses pre-commit alas.
So, you would need to integrate it inside cargo dev generate-all as it centralizes the code generation and the CI would check if the generated code is up to date.

@charliermarsh
Copy link
Member

Thanks for getting involved :) Happy to incorporate this but it does likely need to be part of generate-all.

@Pierre-Sassoulas
Copy link
Contributor Author

Any tip on how to make rule.as_ref() be a link only for pylint's rule ? I'm new to rust I don't see where the function needs to be modified, I think it's generic for all Rules.

@not-my-profile
Copy link
Contributor

Couple of notes:

  1. We do not want to blindly rename our violations to match Pylint. As I said in Human-friendly rule names #1773 we want instead want to follow the Rust naming convention for lints:

    the lint name should make sense when read as "allow lint-name" or "allow lint-name items"

    So use-sys-exit should actually be called use-of-exit-or-quit.

    The documentation of pylint messages only appears to be linkable via the pylint symbolic name ... we do not currently track these ... I think that's very much a blocker for implementing this (unless pylint introduces some redirect endpoint that redirects from the pylint code to the respective documentation).

  2. We certainly want to implement this in a way that also works for other linters.

Any tip on how to make rule.as_ref() be a link only for pylint's rule ? I'm new to rust I don't see where the function needs to be modified, I think it's generic for all Rules.

as_ref is much more generic than that ... since it's an interface from the AsRef trait of the standard library. In this case it just returns a &str.

I am afraid that this is not really a beginner friendly issue, if you're new to Rust, you'll probably have a better experience trying to do something from the issues labeled as good first issue.

@charliermarsh
Copy link
Member

For what it's worth: it's fine to rename the Pylint rules to match upstream for now. We do it for most of them, and that's the current intent for that rule set, so any that deviate are an error. We need to do a pass to rename all rules consistently anyway prior to making that a public API.

In other words: as long as we aren't applying "allow lint-name" consistently across Ruff, then having Pylint rules be consistent with upstream is preferable to having them be inconsistent with upstream. So I'm fine committing those changes. (I won't be working on them personally, but I will certainly merge them.)

@not-my-profile
Copy link
Contributor

not-my-profile commented Feb 4, 2023

I'd rather avoid needlessly naming rules back and forth. Many of the pylint rules are implemented also by flake8 plugins so naming them after pylint is rather arbitrary.

@Pierre-Sassoulas
Copy link
Contributor Author

Maybe we can:

  • Add the links in post-process after the Readme is generated (?) If so I can more or less adapt the python script to rust
  • Have a 1-n relationship between ruff messages and other linter's messages (in the future for pylint, as the renaming was merged in Rename the misnamed pylint messages #2559). That would also be convenient outside of ruff's repository if it's a json or something that can be parsed to generate doc automatically here and elsewhere.

@charliermarsh
Copy link
Member

Let's omit this for now. We now have the ability to write documentation inline (see: #1467 (comment)), so we'll bring these over as first-class documentation in Ruff itself (see: #2646).

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

Successfully merging this pull request may close these issues.

None yet

4 participants