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 CI #463

Closed
CSSFrancis opened this issue Nov 6, 2023 · 4 comments
Closed

Add Pre-commit CI #463

CSSFrancis opened this issue Nov 6, 2023 · 4 comments
Labels
dev Package maintenance
Milestone

Comments

@CSSFrancis
Copy link
Member

CSSFrancis commented Nov 6, 2023

I think it would be nice to add pre-commit CI to run in orix.

It's pretty easy to set up and can be configured to run isort as well as black on the codebase. This can either be automatic or by commenting pre-commit auto fix on some Pull Request. I think this greatly reduces the barrier for contributing to a repository as a first time contributor. While it seems like it should be fairly easy to get pre-commit/ black to work I've run into problems with.

  1. Code that is already committed not being caught by pre-commit
  2. Version mismatch for black
  3. Pre-commit only running black and not isort and failing silently.

I feel like I'm fairly proficient at most things but pre-commit and black give me lots of headaches when I feel like they shouldn't. Maybe I'm alone here and everyone else has no problems with this :)

@hakonanes
Copy link
Member

While initially against this (see #460 (comment)), you've convinced me.

The current code base is in line with our desired formatting (https://github.com/pyxem/orix/blob/develop/.pre-commit-config.yaml). If we set up a pre-commit fix, it should be configured to recognize this.

I'd like to note that I think all listed problems can be fixed without too much effort (?):

  1. Run black and commit (either amend the previous commit or make a new one)
  2. Install version listed in our pre-commit config (https://github.com/pyxem/orix/blob/develop/.pre-commit-config.yaml)
  3. As 1., but run isort and commit

What do you think of these manual solutions?

@hakonanes hakonanes added the dev Package maintenance label Nov 6, 2023
@CSSFrancis
Copy link
Member Author

I'd like to note that I think all listed problems can be fixed without too much effort (?):

@hakonanes Haha you are correct here. It's not like it's the hardest thing to do, sometimes I'm just lazy :)

Anecdotally, I have had to help out a couple of people with black and I feel like making the 1st contribution as easy as possible is usually a good thing. I kind of see these things as necessary evils. I hate black formatting but I hate inconstant formatting more.

@harripj
Copy link
Collaborator

harripj commented Jan 18, 2024

Just wanted to add my two cents here for anybody using VS Code. There are two extensions for black and isort which can be configured to auto-format on save. Personally I find these extensions very useful when developing as one can focus on the logic and forget about the formatting 😄

@CSSFrancis
Copy link
Member Author

Done in #472

@pc494 pc494 mentioned this issue Mar 26, 2024
4 tasks
@pc494 pc494 closed this as completed Mar 26, 2024
@pc494 pc494 added this to the v0.12.0 milestone Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev Package maintenance
Projects
None yet
Development

No branches or pull requests

4 participants