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

Laid out infrastructure for switching to black formatting all code. #3222

Open
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

MicahGale
Copy link
Contributor

@MicahGale MicahGale commented Dec 13, 2024

Description

The goal of this PR is to actually be PEP8, and enforce it. This does so by:

  1. Setting the expectation for using black. Black is best when used universally.
  2. Creating a CI job for checking that black was used for formatting all changes with black --check
  3. Adding black as an optional dependency.

To avoid confusion this PR does not actually apply black, that is done in #3223.

Fixes #2002

Checklist

  • I have performed a self-review of my own code
  • I have run clang-format (version 15) on any C++ source files (if applicable)
  • I have followed the style guidelines for Python source files (if applicable)
  • I have made corresponding changes to the documentation (if applicable)
  • I have added tests that prove my fix is effective or that my feature works (if applicable)

@MicahGale MicahGale marked this pull request as ready for review December 13, 2024 22:40
@MicahGale
Copy link
Contributor Author

Failure of Python-format-test is to be expected, and shows that the test is working.

@shimwell
Copy link
Member

Awesome, particularly keen on this PR as I think it will save a lot of human time reviewing and suggesting minor formatting changes on PRs.

Could also consider changing this line in the PR template

- [ ] I have run [clang-format](https://docs.openmc.org/en/latest/devguide/styleguide.html#automatic-formatting) (version 15) on any C++ source files (if applicable)

@MicahGale
Copy link
Contributor Author

Awesome, particularly keen on this PR as I think it will save a lot of human time reviewing and suggesting minor formatting changes on PRs.

Could also consider changing this line in the PR template

- [ ] I have run [clang-format](https://docs.openmc.org/en/latest/devguide/styleguide.html#automatic-formatting) (version 15) on any C++ source files (if applicable)

Are you thinking of adding "I have ran black" language to this?

@shimwell
Copy link
Member

Are you thinking of adding "I have ran black" language to this?

Yep perhaps good to keep it as a single check box.

Something like

I have run Clang on C++ code changes and Black on Python code changes.

Copy link
Contributor

@lewisgross1296 lewisgross1296 left a comment

Choose a reason for hiding this comment

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

Thoughts on adding the badge Code style: black to the README.md? It's purely cosmetic but could be fun and a way for users to find the project.

Looks like python 3.10.0 did go out of SPEC 0 in October, but some of the other minor versions were released after. For example, I'm still using 3.10.5 locally, and that was released in June 2022. What is the policy on SPEC 0 for minor versions?

@MicahGale
Copy link
Contributor Author

  1. I'll give the badge a shot.

  2. Yes python 3.10 is out of SPEC 0 now. I'm guessing you meant patch version (third digit). I think spec 0 doesn't care about patch version and only cares about minor versions. I have another open PR Migrate to Python 3.13 #3164 for this migration. You should probably upgrade to python 3.13 or 3.12.

@shimwell
Copy link
Member

Is it worth considering using pre commit for black formatting

@MicahGale
Copy link
Contributor Author

Is it worth considering using pre commit for black formatting

I personally prefer to run black manually. I feel like this is more of a personal developer choice.

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.

coding style for Python
3 participants