Skip to content

Add pre-commit and Ruff setup with flake8 and other codestyle checks #2531

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

Merged
merged 3 commits into from
Mar 17, 2025

Conversation

dhomeier
Copy link
Collaborator

@dhomeier dhomeier commented Mar 1, 2025

Description

This PR is adding

  1. a Ruff configuration to allow using ruff check as a linter in place of, among other checks, the current codestyle CI test. The setup is largely following the Astropy example with global and per-file ignores for all checks not passing in the present codebase. Like in Astropy, rules considered not applicable are placed as permanent exemptions in pyproject.toml, while currently failing roles are put in .ruff.toml, to be fixed in the code at a given time.
  2. a pre-commit setup making this Ruff installation available as a local pre-commit hook and as runner for the codestyle job.

For usage I refer again to the Astropy documentation above; basic options for local usage are running

  • pre-commit install to have checks (and where possible fixes) automatically applied on each commit
  • pre-commit run [--all-files] to manually check staged files or the entire repo
  • ruff check to directly run the linting checks with Ruff.

Fixes to the actual files, especially those with per-file exemptions at the end of .ruff.toml can be done in a follow-up step.

@dhomeier dhomeier force-pushed the pre-commit-flake8 branch from 1118647 to d9bfebd Compare March 1, 2025 20:40
@dhomeier dhomeier requested a review from Carifio24 March 5, 2025 20:58
Copy link
Member

@Carifio24 Carifio24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me and seems to be working well. I just had one minor comment (about a comment).

So if I'm understanding correctly, the idea is to put permanent rule exclusions in pyproject.toml, while the ones in ruff.toml are "we should fix these at some point"?

extend = "pyproject.toml"
lint.ignore = [
# NOTE: to find a good code to fix, run:
# ruff --select="ALL" --statistics glue/<subpackage>
Copy link
Member

@Carifio24 Carifio24 Mar 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be ruff check --select="ALL" --statistics glue/<package> (the check is missing currently)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this; yes, probably referred to a deprecated way of invoking ruff with "check" set as default action.

@dhomeier
Copy link
Collaborator Author

So if I'm understanding correctly, the idea is to put permanent rule exclusions in pyproject.toml, while the ones in ruff.toml are "we should fix these at some point"?

Yes, that would be largely following Astropy policy, which at least @astrofrog and I are reasonably familiar with.
The initial exclusions are also largely copied directly over, then just adjusted for areas where glue-core currently shows large-scale violations. Some major additions at this point are:

The last set is now already in pyproject.toml as a permanent exception, but again where to put these or other rules eventually is open to discussion.

@Carifio24
Copy link
Member

Ah yeah, I see now that you had said that in the original PR description too - sorry for not reading more carefully!

That certainly sounds reasonable to me, and if it's worked for Astropy then I imagine it works quite well.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

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

Successfully merging this pull request may close these issues.

3 participants