Select and comply with all Ruff rules, minus a curated ignore list #290
Conversation
CodSpeed Performance ReportMerging #290 will not alter performanceComparing Summary
Footnotes
|
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Thanks for this, it's great.
I would slightly prefer some of the rules to be turned off, let's align in the comment threads. It should be obvious from my tone on each one how strongly I feel (generally, not very!)
src/grimp/adaptors/caching.py
Outdated
| # While we would expect the module to be in here, | ||
| # there's no point in crashing if, for some reason, it's not. | ||
| raise CacheMiss | ||
| raise CacheMiss from None |
src/grimp/adaptors/caching.py
Outdated
| from ..application.ports.caching import CacheMiss | ||
| from grimp import _rustgrimp as rust # type: ignore[attr-defined] | ||
|
|
||
| if TYPE_CHECKING: |
There was a problem hiding this comment.
On the fence on this one - I suppose there is hypothetically a slight runtime penalty, but for me it makes the code a little less readable. What rule is it?
There was a problem hiding this comment.
The TC ruleset from flake8-type-checking, specifically codes TC001, TC002, and TC003.
https://docs.astral.sh/ruff/rules/#flake8-type-checking-tc
The runtime penalty is probably near-zero because import grimp will basically import all submodules, and grimp has no dependencies. So it would only be stdlib imports but I think it's not worth being concerned about those.
I've gone ahead and reverted this from the PR. We can always add it back in later if there's a desire for it, it's easy to migrate with autofix.
| ) | ||
| except rust.NoSuchContainer as e: | ||
| raise NoSuchContainer(str(e)) | ||
| raise NoSuchContainer(str(e)) from None |
There was a problem hiding this comment.
You'd be in a better position to know :)
Probably I should have put that rule in a separate PR for closer review.
Makes sense, I think basically I thought since the str(e) was getting passed to the error that perhaps duplicating it in the traceback could be excessive especially since we'll hit the rust layer pretty soon after
Anyway I've made it from exc (consistent naming with other parts of the codebase).
…for rule ignores
| def __getattr__(name: str) -> Any: | ||
| if name == "NamespacePackageEncountered": | ||
| _NamespacePackageEncountered._warn_deprecated() | ||
| warnings.warn( |
There was a problem hiding this comment.
I'll just note, that Ruff agreed with your intuition in #276 so I've actually refactored it after all :)
…acePackageEncountered`
…factor/enable-strict-ruff-config
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Lovely stuff, thanks for taking the trouble to do this.
| @@ -0,0 +1,58 @@ | |||
| line-length = 99 | |||
There was a problem hiding this comment.
Going the extra mile with all these comments! 👏🏻
I may edit some of them in a subsequent PR as I don't agree with everything here but it's a really useful first cut.
2 participants
Continuing from #282 and #284