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

Ignore fewer flake8 rules, add flake8-bugbear #1133

Merged
merged 9 commits into from
Dec 24, 2019
Merged

Conversation

karalekas
Copy link
Contributor

@karalekas karalekas commented Dec 23, 2019

Description

Follow-up to #1132. Fixes #1109.

The only remaining excluded flake8 rules are:

# E203 : space before ":" (not PEP8 compliant)
# E741 : ambiguous variable name (unfortunately we need to use "I")
# E743 : ambiguous function definition (unfortunately we need to use "I")
# W503 : line break before binary operator (not PEP8 compliant)
ignore = E203, E741, E743, W503

In addition, adds flake8-bugbear which contains some additional rules (the developers of Black use it, and it's otherwise widely used).

Finally, consolidates the style-related Makefile targets / CI jobs under a single naming convention: check-all, check-format, check-style, and check-types.

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 review from kalzoo and a team December 23, 2019 22:43
@karalekas karalekas changed the title Ignore as few of the flake8 rules as is possible Ignore fewer flake8 rules, add flake8-bugbear Dec 23, 2019
@karalekas karalekas added the devops 🚀 An issue related to CI/CD. label Dec 23, 2019
pyquil/gates.py Show resolved Hide resolved
@@ -134,7 +134,7 @@ def test_decoherence_noise():

# verify that gate names are translated
new_prog = apply_noise_model(prog, m3)
new_gates = _get_program_gates(new_prog)
_get_program_gates(new_prog)
Copy link
Contributor

Choose a reason for hiding this comment

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

_get_program_gates doesn't appear to be side-effecty, nix? or else maybe the intention was to do something with new_gates?

Comment on lines 1388 to 1390
less_trivial_pi = "3 * theta[0] * 2 / (pi)"
prog = Program(f"RX({trivial_pi}) 0")
exp = str(prog[0].params[0])
assert _eval_as_np_pi(trivial_pi) == _eval_as_np_pi(exp)
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps the line below was actually meant to RX({less_trivial_pi}) and then _eval_as_np_pi(less_trivial_pi) on line 1391?

Comment on lines 1393 to 1394
more_less_trivial_pi = "3 / (theta[0] / (pi + 1)) / pi"
prog = Program(f"RX({trivial_pi}) 0")
exp = str(prog[0].params[0])
assert _eval_as_np_pi(trivial_pi) == _eval_as_np_pi(exp)
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

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.

Maybe a couple more?

pyquil/tests/test_quil.py Outdated Show resolved Hide resolved
pyquil/tests/test_quil.py Outdated Show resolved Hide resolved
@appleby
Copy link
Contributor

appleby commented Dec 24, 2019

Maybe a couple more?

Hmmm semaphore tests passed, so maybe not!

@karalekas karalekas merged commit f98ec12 into master Dec 24, 2019
@karalekas karalekas deleted the buttery-flaky-crust branch December 24, 2019 17:53
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.

Ignore fewer flake8 style rules
2 participants