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

CI: Add python formatting to pre-commit and make all files comply #398

Merged
merged 5 commits into from
Oct 1, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Sep 25, 2024

BEGINRELEASENOTES

  • Switch the pre-commit action to run in a Key4hep environment
  • Add ruff formatting to pre-commit
  • Fix a few python2 style print statements
  • Fix format of all python files

ENDRELEASENOTES

As discussed in #392 and #396. Given that some of the files still had python2 style print statements, ruff didn't work on all of them without intervention. I have made a separate commit that only fixes those (semi-automated), and then formatted everything afterwards.

@tmadlener tmadlener force-pushed the run-pre-commit-key4hep branch 3 times, most recently from 09272ca to 5048394 Compare September 25, 2024 15:58
@tmadlener
Copy link
Contributor Author

@andresailer should we try formatting with longer lines to reduce a few of the line breaks? We could also configure a linter, but I am not sure how well that deals with all the dynamic imports from DD4hep and/or Gaudi. Plus, I am not sure I want to go fix all the ILD scripts that have probably been unused for quite some time.

@andresailer
Copy link
Contributor

@andresailer should we try formatting with longer lines to reduce a few of the line breaks? We could also configure a linter, but I am not sure how well that deals with all the dynamic imports from DD4hep and/or Gaudi. Plus, I am not sure I want to go fix all the ILD scripts that have probably been unused for quite some time.

Yes, longer line would probably be nice. Linter also, but agreed that it would be annoying to fix unused scripts from years gone by.

We do flake8 in DD4hep https://github.com/AIDASoft/DD4hep/blob/master/.github/workflows/python.yml so that seems to be not too bad, unless I missed a log of ignore statements in our files.

example/ddsim.py Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor Author

I set the line length to 99, mainly because we have the same in podio and there is no other value yet in Key4hep. Diff seems to become slightly smaller, but I suppose the differences are rather minimal wrt default line length of 88.

Copy link
Contributor

@andresailer andresailer left a comment

Choose a reason for hiding this comment

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

OK with 99, it is good at least to keep things consistent with podio.

Any objections by anyone?

@tmadlener
Copy link
Contributor Author

Looks like no one is unhappy enough to complain, so I would suggest to merge this.

@andresailer andresailer merged commit d904a01 into main Oct 1, 2024
7 checks passed
@andresailer andresailer deleted the run-pre-commit-key4hep branch October 1, 2024 06:14
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.

2 participants