Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add isort-related checks to ruff #370

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

dalonsoa
Copy link
Contributor

@dalonsoa dalonsoa commented Feb 26, 2025

And re-run ruff in all files.

There are a lot of things changed that should have been changed when applying ruff the previous time. I'm not entirely sure if those changes were not implemented in full or if there have been additions afterwards modifying that or using a different set of rules (nor ruff). Is everyone using pre-commit hooks? I strongly recommend to do so, to ensure the repo code style is kept.

I've also updated the lint.yml action to be more thorough as I don't think it was running at all, otherwise these would have been catch.

Fixes ImperialCollegeLondon#9

@dalonsoa
Copy link
Contributor Author

852c19c is actually running the tool - and all the resulting changes. The other 4 is about updating the configuration.

@plasorak
Copy link
Collaborator

What exactly is the pre-commit hook doing? Is this adding a commit after each PR to run isort?

@jamesturner246
Copy link
Contributor

Hi @plasorak.

Devs set themselves up by running pre-commit install to install the hooks. From then on, pre-commit runs all installed hooks right when calling git commit. If hooks return failure, then the commit is rejected and the dev must fix the issue.

Some, like ruff, are smart enough to automatically make fixing changes. In which case you would just git add again the files auto-fixed by ruff, and then git commit again.

@dalonsoa
Copy link
Contributor Author

@plasorak , as @jamesturner246 says, pre-commit does not change or add commits, but rather ensures that any commit follows whatever standards are defined, preventing the commit from being created until the issues are resolved.

drunc already contains a pre-commit-config.yaml indicating the checks to run on every commit only on the files modified by the commit. In this case, we only run ruff, but in other repositories (eg. in drunc_ui itself) we run lots of stuff, from formatting other file types to spell checking.

This is supported by a properly set CI workflow, which was not in this case. As mentioned in the PR description, this PR fixes that and ensures ruff is run correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable isort checks
3 participants