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

fix(ruff): less pedantic max-line-length config #440

Closed
wants to merge 2 commits into from

Conversation

yajo
Copy link
Contributor

@yajo yajo commented Dec 20, 2023

Until template v5.x, we just used Black, which didn't cry about line
length. After an upgrade to the latest template, there are a lot of E501
errors that are not really helpful. The autoformatter will fix most
cases, but sometimes having a longer line is more readable.

Here, I'm increasing the limit to 120 characters. If it's still too low,
we can just remove E501 from the list of errors.

@moduon MT-4544 @EmilioPascual

Until template v5.x, we just used Black, which didn't cry about line
length. After an upgrade to the latest template, there are a lot of E501
errors that are not really helpful. The autoformatter will fix most
cases, but sometimes having a longer line is more readable.

Here, I'm increasing the limit to 120 characters. If it's still too low,
we can just remove E501 from the list of errors.

@moduon MT-4544
@yajo
Copy link
Contributor Author

yajo commented Dec 20, 2023

@pedrobaeza
Copy link
Member

I prefer not to do that. The limit should be the same 88 cols.

@yajo
Copy link
Contributor Author

yajo commented Dec 20, 2023

Until ruff was introduced, we were using black. Quoting from their docs:

If you’re paid by the line of code you write, you can pass --line-length with a lower number. Black will try to respect that. However, sometimes it won’t be able to without breaking other rules. In those rare cases, auto-formatted code will exceed your allotted limit.

Thus we just accepted that case where longer lines can exist here and there and all is fine.

Why this change of mind now?

@pedrobaeza
Copy link
Member

It's not a change of mind. It's just that the line length was linted by flake8 (or that was the intention, like in OCA), with the bugbear extension that extends a 10% percent of the 80 cols (88).

@yajo
Copy link
Contributor Author

yajo commented Dec 20, 2023

No, we didn't do that here.

@pedrobaeza
Copy link
Member

That's an anomaly from OCA, so I prefer to stick to it (in fact, I thought it was activated, so that wasn't done on purpose), and as you have said, most of the times, black does its job, and you won't find too many errors due to this new linter. Other new linters may be more annoying than this one and you have needed to adapt them (like removing string from tree), so let's keep this good practice on length.

@yajo
Copy link
Contributor Author

yajo commented Dec 21, 2023

Ok, nevermind, we'll configure this on our own repos.

@yajo yajo closed this Dec 21, 2023
@yajo yajo deleted the more-ruff branch December 21, 2023 07:54
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.

3 participants