enable stricter ruff configuration #1982
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1982 +/- ##
==========================================
- Coverage 97.95% 97.95% -0.01%
==========================================
Files 53 53
Lines 5581 5575 -6
==========================================
- Hits 5467 5461 -6
Misses 114 114
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Thanks so much @danieleades for opening a new PR this quick 🚀
LGTM.
I'm really not a fan of E501, especially with 80-characters width, which is quite short and triggers a lot of those, but let's not address this here 👍
If you feel that way, it makes sense to increase that character limit before merging this PR since the lines will wrap differently. Do you have a specific width in mind? |
|
I generally use 120. Let see what other maintainers think. |
|
I have don't have a strong opinion on line length, but I tend to prefer sticking with (sane) defaults. Ruff defaults to 88. Do you feel it's a too low value? |
|
With 4-space indents, I think it's too low, yes. There are many, many strings and snippets that get split or exploded on multiple lines because we're in a condition in a loop in a method in a class, and it feels like the code get squished. But I understand why people sometimes prefer sticking to 80/88, as it makes it easier to read code side by side in multiple panes. So, no strong opinion. If none of us has strong opinions, lets keep the current line length 🙂 |
danieleades
force-pushed
the
ruff-stricter2
branch
from
fccfefa to
02a8c9c
Compare
There was a problem hiding this comment.
@danieleades Looks great! Just some minor remarks from my side.
| class CopierAnswersInterrupt(CopierError, KeyboardInterrupt): | ||
| class CopierAnswersInterruptError(CopierError, KeyboardInterrupt): |
There was a problem hiding this comment.
I think we should add an alias
# Backwards compatibility
CopierAnswersInterrupt = CopierAnswersInterruptErrorto retain backwards compatibility, just in case somebody relies on catching this exception.
There was a problem hiding this comment.
I agree, good catch @sisp.
| @@ -69,7 +69,7 @@ def test_answer_changes( | |||
| with local.cwd(src): | |||
| build_file_tree( | |||
| { | |||
| "{{ _copier_conf.answers_file }}.jinja": "{{ _copier_answers|to_nice_yaml }}", | |||
| "{{ _copier_conf.answers_file }}.jinja": "{{ _copier_answers|to_nice_yaml }}", # noqa: E501 | |||
There was a problem hiding this comment.
I think we could avoid such rule exceptions:
| "{{ _copier_conf.answers_file }}.jinja": "{{ _copier_answers|to_nice_yaml }}", # noqa: E501 | |
| "{{ _copier_conf.answers_file }}.jinja": ( | |
| "{{ _copier_answers|to_nice_yaml }}" | |
| ), |
Several more cases like this below.
No description provided.