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

Use Black for code style and enforce in the CI #1132

Merged
merged 19 commits into from
Dec 23, 2019
Merged

Conversation

karalekas
Copy link
Contributor

@karalekas karalekas commented Dec 23, 2019

Description

Often I find myself nitpicking style-related thing in pull requests. No longer! We are going to use Black to format all the Python code in pyQuil, and start enforcing that style in the CI. The only difference from Black's default behavior is that we will continue to use a line length of 100, but that will now be checked as part of make style (i.e. configured in the .flake8 file).

There will be a follow-on PR to peel away the remaining ignored flake8 rules (see #1109).

Checklist

  • The above description motivates these changes.
  • All new and existing tests pass locally and on Semaphore.
  • (New Feature) The docs have been updated accordingly.
  • The changelog is updated, including author and PR number (@username, gh-xxx).

@karalekas karalekas added the quality 🎨 Improve code quality. label Dec 23, 2019
@karalekas karalekas added this to the v2.16 milestone Dec 23, 2019
@karalekas karalekas requested a review from a team as a code owner December 23, 2019 18:58
@karalekas karalekas requested review from a team and kalzoo December 23, 2019 18:58
Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

Not gonna lie: I only eyeballed 34/100 of the files.

Personally I would like a make checkall target that runs make test typecheck formatcheck style and anything else useful, in order to have a single command that can be run after making changes. Or maybe test should be excluded, idk.

Also might be worth grepping for instances of the weird multiline-string-to-single-line case where black didn't actually fuse the strings and left a "...blah blah" "blah..." or "...blah blah" f"blah ..." or else just file it under W for "who cares"

conftest.py Outdated Show resolved Hide resolved
pyquil/api/_compiler.py Outdated Show resolved Hide resolved
pyquil/api/_compiler.py Outdated Show resolved Hide resolved
pyquil/api/_devices.py Show resolved Hide resolved
Copy link
Contributor

@notmgsk notmgsk left a comment

Choose a reason for hiding this comment

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

gosh

@karalekas
Copy link
Contributor Author

karalekas commented Dec 23, 2019

Well there are certainly some interesting choices made by Black in this PR. We could fall back on just checking line length and peeling back all the flake8 exclusions, but that doesn't 100% solve the problem. Things like double vs single quotes and other nuanced style decisions will still end up being annoying parts of PR review. I personally am in favor of going with this (widely used, highly recommended) standard despite some of the strange things highlighted above. If we really took the time to read through the pyQuil source, I'm sure we'd find just as many strange developer-made decisions, but now at least they now follow some uniform guidelines. If we find Black to be overwhelming the development process, we can always disable the formatcheck job in the future, and fall back to just checking flake8-related things, but I think it's worth giving it a shot.

@appleby
Copy link
Contributor

appleby commented Dec 23, 2019

I can't tell yet whether I will love it or hate it, but I'm willing to try new things.

Copy link
Contributor

@kalzoo kalzoo left a comment

Choose a reason for hiding this comment

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

Love it. Haven't used black before, but all the changes I saw in a random sampling here check out as readable and reasonable - and since black verifies the AST, there's no risk here. I think this will make life better for both @karalekas and future contributors, and let us focus on the things that matter here rather than parenthesis indentation.

@notmgsk
Copy link
Contributor

notmgsk commented Dec 23, 2019

I approve in spirit, owing to @kalzoo's positivity. I haven't really reviewed the changes though, and probably won't.

Copy link
Contributor

@appleby appleby left a comment

Choose a reason for hiding this comment

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

I for one welcome our new robot formatting overlords

@karalekas karalekas merged commit 7210fbe into master Dec 23, 2019
@karalekas karalekas deleted the code-style-black branch December 23, 2019 21:52
@karalekas karalekas added the devops 🚀 An issue related to CI/CD. label Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops 🚀 An issue related to CI/CD. quality 🎨 Improve code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants