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

Implement black and isort-based style checks #549

Merged
merged 4 commits into from
Mar 23, 2023
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Mar 22, 2023

This PR adds black and isort style formatting and checking, and requires future code changes to comply with black and isort styles.

  • New workflow check-style.yml that runs black, isort and flake8 checks
  • Removes flake8 check from test workflow
  • etstool.py support for both checking and fixing style, via new style command (with subcommands fix and check)
  • removed old flake8 command from etstool.py
  • changes the max line length setting from 79 to 88 (we can argue about this one) [EDIT: changed back to 79 following internal discussion]

Note: I'm installing the style-related packages from PyPI in etstool.py, so that etstool.py and the new workflow are installing the same package versions. I'm also fixing black to version 23.x so that we don't suddenly get style failures for the first Black release in 2024 (or later years) - see Black's stability policy here: https://black.readthedocs.io/en/stable/the_black_code_style/index.html#stability-policy

To do:

  • Check that the style checks fail as expected (since we haven't fixed the style in the codebase)
  • Make a single commit to apply the style fixers
  • Check that the style checks now pass

Closes #527

@mdickinson
Copy link
Member Author

This is ready for review.

For reviewers: there are two commits. The first commit contains the manual changes; the second is entirely auto-generated using python etstool.py style fix.

@mdickinson mdickinson marked this pull request as ready for review March 22, 2023 14:36
@mdickinson
Copy link
Member Author

For the record, I don't have strong feelings on the line length change here - if the consensus is that people would prefer not to change this, I'll revert and regenerate the changes.

@@ -1,4 +1,5 @@
[flake8]
max-line-length = 79
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like the max is still 79?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I changed it back following discussion in other channels.

Copy link
Contributor

@corranwebster corranwebster left a comment

Choose a reason for hiding this comment

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

Can isort do sorting with lowercase after captials?

One comment about line lengths in flake8.

Not going to argue on the line-length thing - I'm tempted to say just take whatever default black has and that way it'll track whatever "community standards" are going forward.

Nothing else looks out-of-place after a quick skim of the diff

@mdickinson
Copy link
Member Author

Can isort do sorting with lowercase after captials?

It can - there's an order_by_type option. I tend to turn it off because I find the results are too surprising.

@mdickinson
Copy link
Member Author

This will interfere with other open PRs, so I plan to merge the other PRs first (not including the changelog) PR and then redo the automated style fixes if necessary.

@mdickinson
Copy link
Member Author

I did an evil force push to bring this up to date with main, since there didn't seem to be any sane way of merging; I've cherry-picked the relevant two commits (the non-autogenerated ones) and then regenerated the automated fixes on top using python etstool.py fix.

However, I ran into a snag: the current version of black will reformat some existing line-length-compliant docstrings in such a way that they introduce new line length violations, so I had to make manual fix-ups to those docstrings to get them into a state where (a) they passed the line length check, and (b) black won't reformat them to something that no longer passes the line length check.

Upstream PR that introduced this issue: psf/black#3430
See particularly this comment: psf/black#3430 (comment)

@mdickinson mdickinson merged commit 57d5974 into main Mar 23, 2023
@mdickinson mdickinson deleted the black-and-isort branch March 23, 2023 15:48
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.

Consider applying black and isort
2 participants