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

Feature/pre commit pyupgrade #413

Merged
merged 6 commits into from
Nov 10, 2020
Merged

Feature/pre commit pyupgrade #413

merged 6 commits into from
Nov 10, 2020

Conversation

bdice
Copy link
Member

@bdice bdice commented Nov 8, 2020

Description

I found a pre-commit hook called pyupgrade. It modernizes Python syntax. I configured it for 3.6+. I experimented with the tool and it seems to do a nice job, so I thought I would open a draft PR for discussion about whether this tool should be adopted. I reviewed its suggestions and all are valid.

Motivation and Context

Pro: Helps encourage good code style for the project.
Con: Another tool that could break.

Types of Changes

  • New feature

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

@bdice bdice added the enhancement New feature or request label Nov 8, 2020
@bdice bdice self-assigned this Nov 8, 2020
@bdice bdice force-pushed the feature/pre-commit-pyupgrade branch from bdf3b7a to 38e7fd8 Compare November 8, 2020 01:43
@codecov
Copy link

codecov bot commented Nov 8, 2020

Codecov Report

Merging #413 (38e7fd8) into master (15df258) will increase coverage by 0.03%.
The diff coverage is 71.23%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #413      +/-   ##
==========================================
+ Coverage   76.41%   76.44%   +0.03%     
==========================================
  Files          45       45              
  Lines        7139     7123      -16     
==========================================
- Hits         5455     5445      -10     
+ Misses       1684     1678       -6     
Impacted Files Coverage Δ
signac/contrib/filterparse.py 84.12% <0.00%> (ø)
signac/contrib/migration/__init__.py 87.71% <ø> (ø)
signac/contrib/mpipool.py 0.00% <0.00%> (ø)
signac/common/connection.py 38.27% <33.33%> (ø)
signac/contrib/utility.py 58.43% <33.33%> (-0.50%) ⬇️
signac/contrib/import_export.py 85.25% <41.66%> (+0.45%) ⬆️
signac/__main__.py 80.46% <66.66%> (ø)
signac/contrib/filesystems.py 48.57% <66.66%> (+0.45%) ⬆️
signac/contrib/linked_view.py 87.75% <66.66%> (+0.50%) ⬆️
signac/contrib/indexing.py 75.53% <73.07%> (-0.29%) ⬇️
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15df258...38e7fd8. Read the comment docs.

@csadorf
Copy link
Contributor

csadorf commented Nov 9, 2020

What's the difference to a standard linter? It doesn't just point out the issue, but also fixes it? I know many projects that have pre-commit hooks that actually run some automated code improvement prior to the commit, it usually speeds up the workflow.

@bdice
Copy link
Member Author

bdice commented Nov 9, 2020

@csadorf Good questions. Here’s the summary I am planning to give at dev meeting today:

Yes, this hook modifies code (like black, unlike flake8). Pre-commit always runs on the git staging area but never modifies the staging area. All modifications by pre-commit hook(s) become unstaged changes, which can be added.

This hook does not overlap with any other hook we currently use, as it is purely about modernization (its changes do not affect correctness). I believe all of the changes made the code simpler and more readable.

As I have become more familiar with pre-commit and linters more generally, I now have a relatively strong preference for linters and analyzers that can fix things automatically, like black and this hook for pyupgrade. I find that writing code is much faster when I can just type the lines I want and not worry about the whitespace. Let a formatter (clang-format in C++, black or yapf in Python) handle the rest. Moreover, I think hooks that automatically apply their fixes can vastly speed up PR reviews and allow a greater focus on aspects like correctness, usability, and performance (because a high standard for code style is automatically enforced).

@vyasr
Copy link
Contributor

vyasr commented Nov 9, 2020

Yes, I'm a strong proponent for auto-formatters. As long as they are only format changes and guaranteed not to alter the code semantics it saves lots of wasted review iterations and ensures high code quality.

@bdice bdice marked this pull request as ready for review November 9, 2020 17:49
@bdice bdice requested review from a team as code owners November 9, 2020 17:49
@bdice bdice requested review from kidrahahjo and yuanzhou0827 and removed request for a team November 9, 2020 17:49
@bdice
Copy link
Member Author

bdice commented Nov 9, 2020

In dev meeting, we agreed that pyupgrade seems like a good tool and that we would also like to move towards adopting black in the future.

@kidrahahjo
Copy link
Collaborator

@bdice one question? Did this all automatically got updated or did you have to do some sort of refactorings?

@bdice
Copy link
Member Author

bdice commented Nov 9, 2020

@kidrahahjo All of the changes in this PR were performed by pyupgrade. I didn't have to do anything but git add and commit the changes. 😄 I did review all the changes and I think they're all safe.

@bdice bdice closed this Nov 9, 2020
@bdice bdice reopened this Nov 9, 2020
@bdice
Copy link
Member Author

bdice commented Nov 9, 2020

(I accidentally closed this.)

Copy link
Collaborator

@kidrahahjo kidrahahjo left a comment

Choose a reason for hiding this comment

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

Look's great

@bdice bdice merged commit f2d9771 into master Nov 10, 2020
@bdice bdice deleted the feature/pre-commit-pyupgrade branch November 10, 2020 20:41
@bdice bdice added this to the v1.5.1 milestone Dec 19, 2020
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.

4 participants