Use Ruff default ruleset, in addition to our own set - #1228
Conversation
RUF012 (mutable class defaults): we use a lot of class attributes without ClassVar, this change is not justified for now DTZ (naive datetime usage): taskwarrior does use naive datetimes. We can consider using switching to timezone-aware datetimes everywhere but that's a bigger change than just applying some linting rules.
.lstrip('Patch Set ') strips any leading characters in that set rather
than the literal prefix, silently mangling comment text that happens to
start with overlapping letters (e.g. "this is a message" became "is is a
message"). removeprefix() only strips an exact match.
Covers F401 (unused import), PLE0604 (non-literal __all__ entries), TRY002 (raise a specific exception instead of bare Exception), TRY401 (drop exception text already carried by log.exception's traceback), SIM102/SIM113/SIM117 (simplifications), UP045 (Optional -> X | None), PERF402 (append-loop -> .extend()), and UP031 (percent-format -> f-strings). Also fixes a real bug surfaced by UP031 in db.py: `'...' % exit_code, hook` built a tuple due to operator precedence instead of formatting both values, so run_hooks() would raise TypeError instead of logging whenever a hook exited non-zero.
Two calls are left as-is since the template itself is resolved at runtime, not a literal: schema.py's self.KEYRING_SERVICE.format(**self.model_dump()) and github.py's path.format(**context).
ryneeverett
left a comment
ryneeverett
left a comment
There was a problem hiding this comment.
Why wasn't .format() -> f-string caught by UP032?
|
It looks like UP032 does not handle I migrated everything for consistency sake |
ryneeverett
left a comment
ryneeverett
left a comment
There was a problem hiding this comment.
I'd like you to drop 5de1539. Otherwise this seems good to go.
Style standards which are not enforced by formatters or even linters are problematic because they open the door to thinking or quibbling about style, the avoidance of which is the main point of these tools.
I think UP032 overstates the case against .format() when they say:
f-strings are more readable and generally preferred over
str.formatcalls.
If you look at pyupgrade from which this rule was derived, they only give examples of clear readability improvements and say:
note: pyupgrade is intentionally timid and will not create an f-string if it would make the expression longer or if the substitution parameters are sufficiently complicated (as this can decrease readability).
I would give the first modification in bugwarrior/services/__init__.py as an example of a readability degradation:
desc_len = self.main_config.description_length
- return "(bw){}#{} - {}{}{}".format(
- cls_markup.get(cls, cls.title()),
- number,
- title[:desc_len] if desc_len else title,
- url_separator if url else '',
- url,
+ title_text = title[:desc_len] if desc_len else title
+ return (
+ f"(bw){cls_markup.get(cls, cls.title())}#{number} - {title_text}"
+ f"{url_separator if url else ''}{url}"
)I'd be fine with many of the format -> f-string switches here but the motivation would be readability on a case-by-case basis and not consistency.
2 participants
ruff recently released v0.16
While looking at the changelog, I realized we use
selectinstead ofextend-select, so it overrides any default rule configured by ruff. This switches toextend-select, so we benefit from the default set of rules.I chose to ignore some rules:
I also noticed two real existing bugs:
bugwarrior/db.py(run_hooks): the error message expected 2 inputs, while only 1 was provided.bugwarrior/services/gerrit.py(annotations):.lstrip('Patch Set ')was used instead ofremoveprefix, removing any matching char (e.g. "this is a message" became "is is a message")In the last commit, I removed the "quote-style" ruff setting, to use the default one, so we have a slightly more consistent codebase. I can remove this commit if we don't want that change for now.