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

Reformat files with Black #233

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

syclops
Copy link

@syclops syclops commented Mar 10, 2024

For consistent formatting, run all Python sources files (.py) and notebooks (.ipynb) through Black. To ensure that configuration options (e.g., line length) remain consistent across different collaborators, also add a pyproject.toml configuration file with relevant options.

Note that the current configuration for Black in pyproject.toml uses the preview feature for better multiline string formatting. The tradeoff is that it also enables other experimental features that are not guaranteed to be consistent across multiple versions of Black. If Pylint checks are added in the future, the preview option can probably safely be removed.

No new tests were added, as this is a feature that doesn't change the behavior of the code in any way. This is purely a formatting change, with an added configuration. That being said, I'm happy to add some sort of checks for Black formatting in the future if this PR is helpful.

For consistent formatting, run all Python sources files (.py) and
notebooks (.ipynb) through Black. To ensure that configuration options
(e.g., line length) remain consistent across different collaborators,
also add a pyproject.toml configuration file with relevant options.

Note that the current configuration for Black in pyproject.toml uses the
preview feature for better multiline string formatting. The tradeoff is
that it also enables other experimental features that are not guaranteed
to be consistent across multiple versions of Black. If Pylint checks are
added in the future, the preview option can probably safely be removed.
@zdelrosario
Copy link
Owner

Thanks @syclops ! I love the idea of adding consistent formatting.

Do you have any suggestions on how to maintain this formatting in future development? Are there pre-commit hooks we can implement (maybe add a recipe to the Makefile to help new developers set this up)? Or is there even a way to automate this formatting in our GitHub Actions?

@zdelrosario zdelrosario self-assigned this Mar 11, 2024
@zdelrosario zdelrosario added the enhancement New feature or request label Mar 11, 2024
@syclops
Copy link
Author

syclops commented Mar 30, 2024

@zdelrosario Sorry for the delay.

It is possible to automatically apply the formatting in GitHub Actions, but it's not recommended - doing so would create an extra commit that then needs to be synced up by the PR author.

The pre-commit hook is nice because (1) it can be easily run locally, and (2) the built-in hooks let you check for a lot more than Black formatting (see here for a list of checks: https://github.com/pre-commit/pre-commit-hooks#hooks-available). It's also reasonably straightforward to add to the repo: https://black.readthedocs.io/en/stable/integrations/source_version_control.html

If you'd like to use pre-commit hooks, it's probably worth updating contributing.md to tell contributors that they'll need the pre-commit tool.

Since pre-commit hooks are only local, they can be overridden, and it may also be good to set up a GitHub Action to check formatting with Black. That would involve adding a workflow file like what's seen here: https://black.readthedocs.io/en/stable/integrations/github_actions.html

I'm happy to add these if you want - just let me know and I'll update the PR.

Finally, I noticed that the existing checks aren't passing, but don't seem to be due to formatting changes. If these failed tests were caused by reformatting with Black, let me know and I'll be happy to look into it.

@zdelrosario
Copy link
Owner

@syclops no worries!

Apologies about the build fail you saw; that's a me problem. I've resolved that in a recent update to master, which you can pull into your fork.

I agree with what you propose: The combination of pre-commit hook with an Action to check (but not commit) would make this a sustainable change. I also like the idea of editing the contributing.md file! Many thanks for offering to make these useful changes.

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.

2 participants