Enable various Ruff rulesets #284
Conversation
| "DirectImport", | ||
| "Import", | ||
| "ImportGraph", | ||
| "Layer", |
There was a problem hiding this comment.
This is sorting __all__ alphabetically
| tails=frozenset( | ||
| { | ||
| "mypackage.application.7537183614.6928774480.5676105139.3275676604" | ||
| # noqa:E501 |
There was a problem hiding this comment.
The latest version of ruff doesn't consider these to be "line-too-long" violations so these rule codes can be removed.
| components = [package_name] + internal_filename_and_path_without_extension.split( | ||
| self.file_system.sep | ||
| ) | ||
| components = [ |
There was a problem hiding this comment.
This unpacking syntax is a stylistic choice. Personally I like it but feel free to disagree - I'll revert and ignore the rule.
| graph = ImportGraph() | ||
|
|
||
| with pytest.raises(ValueError, match="Module foo is not present in the graph."): | ||
| with pytest.raises(ValueError, match=re.escape("Module foo is not present in the graph.")): |
There was a problem hiding this comment.
match= arguments are regex (which is easy to forget), so characters like . have special meaning. This can lead to subtle bugs. Better to explicitly escape the string.
| one_chain = (source, a, b, c, destination) | ||
| other_chain = (source, d, e, f, destination) | ||
| assert (result == one_chain) or (result == other_chain) | ||
| assert result in (one_chain, other_chain) |
There was a problem hiding this comment.
Stylistic (similar comment to before regarding unpackaging syntax)
| for layer in layers: | ||
| assert len(layer.module_tails) == 1 | ||
| module = list(layer.module_tails)[0] | ||
| module = next(iter(layer.module_tails)) |
There was a problem hiding this comment.
Stylistic (similar comment to before regarding unpackaging syntax)
It's a slight performance hit to instantiate the list in full, so that's the motivation for this slightly less readable syntax. Perhaps should be disabled for the test suite
|
|
||
| # Legal imports, from higher layer to immediate lower layer | ||
| for higher_module, lower_module in zip(modules[:-1], modules[1:]): | ||
| for higher_module, lower_module in itertools.pairwise(modules): |
There was a problem hiding this comment.
Stylistic (similar comment to before regarding unpackaging syntax)
I think this one is more readable
| ("import namespace.foo.green.alpha.one", "namespace.foo.green"), | ||
| ("from namespace.foo.green.alpha import one", "namespace.foo.green"), | ||
| ("from ..green.alpha import one", "namespace.foo.green"), | ||
| ("from .. import green", "namespace.foo.green"), |
There was a problem hiding this comment.
These parametrizations are duplicated.
CodSpeed Performance ReportMerging #284 will not alter performanceComparing Summary
Footnotes
|
seddonym
left a comment
seddonym
left a comment
There was a problem hiding this comment.
Looks great.
For auto-generated changes it's fine to bundle them all up together into a commit, as you have done. I am likely to side with ruff on pretty much everything. If you want to turn on an even stricter mode I can review all in one go if you like.
To access autofixes with ruff check . --fix, I wonder whether it would be helpful to configure Just to run Ruff with --fix, if not by default then at least with opt-in for specific use cases, e.g. for within pre-commit
Maybe - feel free to put together a demo PR. At the moment you can do just autofix-python.
2 participants
Another testing the waters sort of thing, I'm wanting to make sure:
I've included rulesets which are relatively uncontroversial (thus mostly already complied with) and have autofixes enabled. I've ignored specific rules which are too controversial.
To access autofixes with
ruff check . --fix, I wonder whether it would be helpful to configure Just to run Ruff with--fix, if not by default then at least with opt-in for specific use cases, e.g. for within pre-commit.