feat: Configure ruff linting and formatting #5
Conversation
Allow ruff to discover all files, instead of hardcoding every path which can be error prone, e.g. including "tests/" in the list meant that nested Python files were not being linted, while other paths were stale. There is no need for this list since we truly want to lint all Python files, which ruff does by default. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop the explicit ruff.format configuration from the pyproject.toml file since it corresponds to ruff's default values, making it harder for someone familiar with the tool to tell deviations from the default behaviour. Given that ruff strives to maintain compatibility with black, these defaults are unlikely to change. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop configuration settings that match ruff's default values (also see previous commit for the changes in ruff.format). Signed-off-by: Phoevos Kalemkeris <[email protected]>
Drop E501 and E226 from ruff.lint's ignore list: * E226 does not appear in the codebase therefore there's no reason to include it there * Given that our line-length limit is already high, we shouldn't ignore E501 Ignoring C901 is not a good idea; the only reason I'm leaving it in is because it would require a lot of refactoring to fix it. We should address this in the future. Signed-off-by: Phoevos Kalemkeris <[email protected]>
Signed-off-by: Phoevos Kalemkeris <[email protected]>
Imports are highly inconsistent throughout the project, using a mix of absolute as well as explicit and implicit relative imports. For this reason, we have to explicitly specify the known first-party and local folders for isort to work correctly. Signed-off-by: Phoevos Kalemkeris <[email protected]>
I strongly believe that the limit should be reduced, but feel free to drop this commit if you disagree. Signed-off-by: Phoevos Kalemkeris <[email protected]>
There was a problem hiding this comment.
By checking the failed CI, I can see tons of errors related to the re-configured maximum line length and unsorted imports. It is not easy to gauge the overall impact of final reformatting changes by reviewing ruff configuration update only. Please consider combining this and the actual reformatting into a single PR.
Run 'ruff check --fix' followed by 'ruff format' and resolve remaining errors manually. For the most part, the manual changes involved one of the 3 following actions: * Split long strings into multiple lines wrapped with parentheses * Refactor code, e.g. retrieve dict values and evaluate conditionals before using the results in f-strings * Move long strings to files (this can be done in many more places around the codebase) Signed-off-by: Phoevos Kalemkeris <[email protected]>
phoevos
force-pushed
the
feature-phoevos-configure-ruff
branch
from
aac43a6 to
88c0b38
Compare
There was a problem hiding this comment.
Looks great from my side 👍. I don't have much of an opinion here to be honest and my guess is this really comes down to the taste of the main contributors (Xi / Phoevos). Looks like most of repos in the cogstack org have > 120 chars per line but this looks pretty neat here.
There was a problem hiding this comment.
The change due to shortening from 120 to 100 characters by adding newlines and sorting the imports is still eye-watering to me. Other change such as removing trailing whitespace and grouping imports make sense (so more specific rules like I003 should be used). Whether to add space to inline arithmetic operators is again down to Devs' taste and could be very subjective.
For this massive reformatting PR, I would suggest a mob review involving more experienced Devs of core CogStack repos, before the discussion here turns into a rabbit hole.
Update
ruffconfiguration inpyproject.toml.More information on the specific changes can be found in the individual commits and we can discuss further if you have any objections or concerns.
Some of the changes are related to bugs (e.g. directories not included in linting), others are additions of linting tools (i.e. isort), while others might be more subjective like the line length limit.