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

Adopt autoflake and pyupgrade formatters #1059

Merged
merged 3 commits into from
May 29, 2019

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented May 20, 2019

In the spirit of #220, here's some development tooling we've adopted in Hypothesis lately for your consideration. I was going to write up an issue, but it seemed easier to explain by example 😉

Both autoflake and pyupgrade aim to canonicalise source code; that is to ensure that equivalent constructs are represented in the same way wherever possible. They also remove some redundancies! For example, autoflake removes unused (standard library) imports and pointless pass statements; while pyupgrade standardizes set and dict comprehensions, string formatting, etc.

Personally, I've found the enforced consistency does make reading code somewhat easier! If we want to take this further, isort and black would round it out, but IMO they deserve some discussion first.

Thoughts?

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #1059 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   99.53%   99.53%   -0.01%     
==========================================
  Files         102      102              
  Lines       12346    12316      -30     
  Branches      921      922       +1     
==========================================
- Hits        12289    12259      -30     
  Misses         36       36              
  Partials       21       21
Impacted Files Coverage Δ
trio/_sync.py 100% <ø> (ø) ⬆️
trio/tests/test_wait_for_object.py 100% <ø> (ø) ⬆️
trio/_core/_unbounded_queue.py 100% <ø> (ø) ⬆️
trio/_subprocess_platform/__init__.py 100% <ø> (ø) ⬆️
trio/_core/_traps.py 100% <ø> (ø) ⬆️
trio/_core/_io_windows.py 91.51% <ø> (-0.08%) ⬇️
trio/_subprocess.py 100% <ø> (ø) ⬆️
trio/_windows_pipes.py 100% <ø> (ø) ⬆️
trio/tests/test_exports.py 96.22% <ø> (-0.07%) ⬇️
trio/tests/test_threads.py 100% <ø> (ø) ⬆️
... and 15 more

@codecov
Copy link

codecov bot commented May 20, 2019

Codecov Report

Merging #1059 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1059      +/-   ##
==========================================
- Coverage   99.53%   99.53%   -0.01%     
==========================================
  Files         102      102              
  Lines       12346    12316      -30     
  Branches      921      922       +1     
==========================================
- Hits        12289    12259      -30     
  Misses         36       36              
  Partials       21       21
Impacted Files Coverage Δ
trio/_sync.py 100% <ø> (ø) ⬆️
trio/tests/test_wait_for_object.py 100% <ø> (ø) ⬆️
trio/_core/_unbounded_queue.py 100% <ø> (ø) ⬆️
trio/_subprocess_platform/__init__.py 100% <ø> (ø) ⬆️
trio/_core/_traps.py 100% <ø> (ø) ⬆️
trio/_core/_io_windows.py 91.51% <ø> (-0.08%) ⬇️
trio/_subprocess.py 100% <ø> (ø) ⬆️
trio/_windows_pipes.py 100% <ø> (ø) ⬆️
trio/tests/test_exports.py 96.22% <ø> (-0.07%) ⬇️
trio/tests/test_threads.py 100% <ø> (ø) ⬆️
... and 15 more

@Zac-HD
Copy link
Member Author

Zac-HD commented May 20, 2019

There's no change in the number of misses, just fewer vacuous hits and I don't think that should be counted as a bad thing ☹️ (hence my crusade against percent coverage as a measure)

@pquentin
Copy link
Member

I like the idea! I also think that we should switch to black to eventually.

But I believe the current plan is to get this proposal accepted: https://forum.bors.tech/t/pre-test-and-pre-merge-hooks/322 so that we can do those things automatically without requiring users to install and run more and more autoformatters, which is frustrating when submitting your first PR.

(Oh, and I think you need to update docs/source/contributing.rst for this PR to be complete.)

@Zac-HD
Copy link
Member Author

Zac-HD commented May 20, 2019

Sounds great!

If others agree, we can take out the bit where this runs in CI, but merge the rest to get most of the benefits - unlike Black, these changes are pretty rare.

@Zac-HD
Copy link
Member Author

Zac-HD commented May 28, 2019

@pquentin - given the CI tooling that we have coming, I've just dropped the CI portion of this PR entirely (beyond comments showing the relevant commands).

That leaves us with a one-off application of pyupgrade and autoflake, which I think is still worth doing - indeed it's almost as good as enforcing it, as e.g. unused imports are quite rare!

@pquentin
Copy link
Member

@Zac-HD For the record, I don't know if we should wait for the CI tooling. Progress seems to be slow, and we're only at the Draft RFC step.

But, yes, the pull request as it is now is much easier to merge since I don't need to gather consensus on whether it's OK or not to continue adding more manual autoformatters.

Thank you!

@pquentin pquentin merged commit 1e95ee4 into python-trio:master May 29, 2019
@Zac-HD Zac-HD deleted the autofixers branch May 29, 2019 05:42
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.

2 participants