Replace all linting and formatting with ruff, improve pre-commit #519
Conversation
|
I assume we can wrap lines to 79 characters with ruff too, right? I think that's one of the things that make it hard to work with the repos effectively - the 200+ long lines. We should pep8 the files properly, incl. to 79 chars. |
|
@xukai92 hi, any objections about moving forward with ruff? I layed out some basics here, but before finishing it up I would like to add some ack from the maintainers that we can continue this way. I do believe ruff can unify and simplify the various tools, it integrates nicely with |
Ruff is fast and it can do isort, black, pylint, flake8 and others. Let's unify on it for the project. It can improve development experience. TODO: - [ ] drop black - [ ] drop pylint - [ ] agree on formatting rules - [ ] resolve all issues found Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
|
@booxter yep, that is easy, if there is an agreement on that. I can enable it to 79 chars. |
|
@booxter are you insisting on 79 chars? while I know it is in PEP8, most of the projects I use relax it a bit, as our modern monitors can usually display, for example we use 99 in most of the projects I work on. Any other suggestions for other maintainers? |
|
@thrix all projects I work with are 79 chars, and honestly using 80x25 sub-terminals to run multiple editor instances side by side is a very common approach. Surprised it's debatable, but of course if others feel otherwise... |
|
@booxter ok, no worries, no need to debate more, thanks |
|
I'm willing to pick up that debate. The default line length for black is 88 characters. Looking at projects that define a line-length in their pyproject.toml, most are at least 88 characters long, while many exceed 100. https://github.com/search?q=pyproject.toml+%22line-length+%3D%22&type=code Myself and my team have settled on around 99 characters. This seems to be the sweet spot between keeping lines reasonably short enough to not wrap/overflow in most editors and not being so short that auto-formatters like black and ruff break up code in ugly ways. I understand the argument for people restricting their editors to 80 characters, but I'm also not sure how common it actually is, on modern monitors. This is largely an issue for people in a split view in vi/vim, which accounts for approximately 5% of Python developers based on the most recent python developer survey. I just took some time to test this out and with two vim editors open in side-by-side tmux panels on a normal 1080 monitor, I can fit 90 characters on a line before it wraps. So even if we wanted to take accommodate this specific case, 88-90 characters is a perfectly reasonable line length. |
|
@JacobCallahan thanks for the input, same here, 99 chars, as 79 is too restricting nowadays for us. Let's wait for others if they have some opinion. Unfortunately going with 79 chars will need quite some amount of edits of the code :( |
| @@ -21,6 +21,39 @@ dynamic = ["dependencies"] | |||
| Homepage = "https://github.com/instruct-lab/cli" | |||
| Issues = "https://github.com/instruct-lab/cli/issues" | |||
|
|
|||
| [tool.ruff] | |||
| select = [ | |||
There was a problem hiding this comment.
Linter selection belongs into section tool.ruff.lint, see https://docs.astral.sh/ruff/configuration/ .
Let's also allow automatic fixing of violations.
| select = [ | |
| [tool.ruff.lint] | |
| # Allow fix for all enabled rules (when `--fix`) is provided. | |
| fixable = ["ALL"] | |
| unfixable = [] | |
| select = [ |
Right now the code is formatted with black and black's default settings. Black defaults to line length 88 characters. I suggest we keep the status quo and configure ruff to use the same line length. Any change to the maximum line length (either 79 or 99) should be discussed, agreed on, and committed in a separate PR. I also recommend to split this PR into four commits, then push the PR without squashing
This allows us to ignore the large-ish auto-format commit with |
|
@tiran thanks for the comments, I will resume my work on this and try to get it into reviewable state this week |
Co-authored-by: Christian Heimes <christian@python.org> Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
Co-authored-by: Christian Heimes <christian@python.org> Signed-off-by: Miroslav Vadkerti <mvadkert@redhat.com>
|
@thrix How about you enable just a few rules for now and do another round of improvements in a second PR? I really, really like to get ruff into this project. It's such a convenient tool and time safer. |
|
@tiran agreed, will try to bake something up finally, thanks for the useful hints |
|
@nathan-weinberg why was this closed without explanation? |
|
@JacobCallahan It was a GitHub bug having to do with the open-sourcing, no action was taken on my part, feel free to reopen |
|
@thrix You may also be in the same position as I am and aren't able to re-open this on your own. |
This limits the taxonomy folders to approval only by `taxonomy-approvers` since later matches take precedence over earlier matches. We then do not need lines for `/docs/` and `*.md` for individuals since they are members of `taxonomy-maintainers` and are thus covered by the first line. This leads to a simpler and easier to maintain file. Signed-off-by: BJ Hargrave <hargrave@us.ibm.com>
Ruff is fast and it can do isort, black, pylint, flake8 and others.
Also enable some standard
pre-committo fix whitespaces, etc.Let's unify on it for the project.
It can improve development experience.
TODO: