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 pre-commit workflow including ruff #207

Merged
merged 14 commits into from
Nov 26, 2024
Merged

Add pre-commit workflow including ruff #207

merged 14 commits into from
Nov 26, 2024

Conversation

schuenke
Copy link
Collaborator

@schuenke schuenke commented Nov 19, 2024

closes #197

Probably nobody wants to review all changes manually, but maybe you can checkout this PR and just check that your last ~10 pypulseq sequences and/or related projects still work fine @FrankZijlstra @btasdelen and maybe even @mavel101 @wtclarke

IMO, this is a necessary step towards a better maintainable package and it will simplify to proceed with #95 and #123

Some infos about pre-commit:

  • I added pre-commit as an optional "test" dependency in pyproject.toml
  • I added a pre-commit GitHub workflow that runs in every PR (and it has to pass to be able to merge the PR)
  • you can download pre-commit from PyPi using pip install pre-commit
  • after downloading, it can be installed using the pre-commit install command
  • when installed, it will run locally every time BEFORE you create a commit. This way you directly know if it will pass the GitHub workflow
  • if you still want to commit something that does NOT pass all pre-commit hooks, you can skip the pre-commit checks using git commit -m "your msg" --no-verify. Note that this does NOT skip the pre-commit GitHub workflow.
  • additional hooks I would like to add in the future: mypy, check-docstring-first

Some further infos about RUFF:

  • RUFF is "an extremely fast Python linter and code formatter"
  • it is used in many large projects like FastAPI, scipy, pandas, ...
  • it supports over 800 different rules
  • it supports a hierarchical configuration (different configs for subfoldes like tests or seq_examples)
  • editor integrations for VsCode and my other editors are available (I highly recommend it!)
  • I already added some additional rules that I would like to enforce in the future. These are commented out in RUFF section of the pyproject.toml file.

@schuenke schuenke added the enhancement New feature or request label Nov 19, 2024
Copy link
Collaborator

@btasdelen btasdelen 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. I tested with a couple sequence, did not see any issues.

@schuenke
Copy link
Collaborator Author

schuenke commented Nov 21, 2024

I also added the config for pre-commi ci now. This allows automated fixes in PRs and will check monthly for updates of the different pre-commit hooks that we use. If we want to activate it, @sravan953 hast to login at https://pre-commit.ci/ and add an installation for pypulseq (just select the repo - thats all).

11ad246 is a good example that pre-commit.ci would have fixed automatically 👆

@schuenke
Copy link
Collaborator Author

@FrankZijlstra are you fine with merging it? Or you wanna have some extra time to review it as well?

@FrankZijlstra
Copy link
Collaborator

I had a quick look and though I'm not familiar with RUFF and how to set it up, from looking at the style changes in the files I think it looks good. Maybe it would be a good idea to add a note to the readme about how to set up pre-commit (basically what you put here)?

I haven't tested it with my sequences, but looking at the changes I don't see anything that would break. So I'm fine with committing this now.

If I remember correctly I still have a branch where I added all the sequence examples to the test_sequence unit tests. And then it shouldn't be too hard to put the new tests in a PR soon(ish), and we can check a bit more elaborate set of sequences automatically.

@schuenke schuenke merged commit 76d9309 into dev Nov 26, 2024
12 checks passed
@schuenke schuenke deleted the precommit branch November 26, 2024 07:55
This was referenced Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define common code style (Linting/Formatting)
3 participants