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 Python formatting targets (black, flake8) #544

Merged
merged 6 commits into from
Jan 30, 2024

Conversation

graeme-a-stewart
Copy link
Contributor

Add CMake targets for running black and flake8 on Python source files

CMake will look to find the relevant binaries and setup the targets if they are there
If they are not found it's a warning (too severe?)
Note that current Key4hep nightlies don't have flake8 - if we use another linter that can be substituted

BEGINRELEASENOTES
Add CMake targets for running black and flake8 on Python source files
ENDRELEASENOTES

Copy link
Collaborator

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Very nice, thanks.

I would demote the warnings to STATUS and also remove the black and flake8 target from all (if they are part of it), simply to not interfere too much with editor plugins or changing files unexpectedly just from running the build.

@tmadlener tmadlener mentioned this pull request Jan 23, 2024
2 tasks
@graeme-a-stewart
Copy link
Contributor Author

I would demote the warnings to STATUS and also remove the black and flake8 target from all (if they are part of it), simply to not interfere too much with editor plugins or changing files unexpectedly just from running the build.

  • Message to status
  • Exclude from all target

Thanks for the suggestions!

@graeme-a-stewart
Copy link
Contributor Author

I think the CI test failure has nothing to do with this PR, right?

https://github.com/AIDASoft/podio/actions/runs/7638050555/job/20808138304?pr=544#step:4:295

@graeme-a-stewart
Copy link
Contributor Author

Once this PR is deemed to be in good shape, I will replicate it for https://github.com/key4hep/EDM4hep.

@tmadlener
Copy link
Collaborator

tmadlener commented Jan 24, 2024

I think the CI test failure has nothing to do with this PR, right?

https://github.com/AIDASoft/podio/actions/runs/7638050555/job/20808138304?pr=544#step:4:295

No, it looks like ROOT has changed something in the RNTuple interface. Specifically, I think it's this PR: root-project/root#14391

Edit: keeping track of this in #545

This adds a target for black and flake8, which must be available
in the build environment
In this case issue a warning and do not set the target
Downgrade the message when formatter binaries are missign to STATUS

Exclude the formatting targets from "all"
Actually use the binary that CMake found, rather than searching PATH again
cmake/pythonFormat.cmake Outdated Show resolved Hide resolved
cmake/pythonFormat.cmake Outdated Show resolved Hide resolved
cmake/pythonFormat.cmake Show resolved Hide resolved
@tmadlener tmadlener merged commit 6ff8ff5 into AIDASoft:master Jan 30, 2024
16 of 18 checks passed
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