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

chore: Fix inconsistent black version in precommit #2314

Merged
merged 6 commits into from
Oct 22, 2020

Conversation

aahung
Copy link
Contributor

@aahung aahung commented Oct 20, 2020

Issue #, if available:

Why is this change necessary?

to make black version consistent with appveyor and dev guide. Even better, by only specifying black version in one place , we can reduce our maintenance cost.

How does it address the issue?

  • make black installation managed
  • only specify black version in one place
  • avoid installing black in pre-commit (reuse existing black installed by make init)
  • simplify dev guide

What side effects does this change have?

Did you change a dependency in requirements/base.txt?
If so, did you run make update-reproducible-reqs

Checklist:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@aahung aahung requested a review from jfuss October 20, 2020 18:28
@@ -1,6 +1,6 @@
repos:
- repo: https://github.com/python/black
rev: 19.3b0
rev: 19.10b0
Copy link
Contributor

Choose a reason for hiding this comment

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

Latest is 20.8b1 of black. Should we upgrade to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. In addition, I made a few changes to how we install black and sync appveyor as well. Waiting for AppVeyor's check.

@aahung aahung force-pushed the fix-black-version branch from 2cf8c85 to 1e7b001 Compare October 20, 2020 20:41
@aahung
Copy link
Contributor Author

aahung commented Oct 20, 2020

The black's pre-commit-config.yaml is taken from https://github.com/psf/black/blob/master/.pre-commit-config.yaml

@aahung
Copy link
Contributor Author

aahung commented Oct 20, 2020

This PR proposes a change on how we install black, I would like to get more feedbacks, thanks!


# formatter
black==20.8b1
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we have this effectively defined, instead of relying on user to have a version of black installed. This has caused conflicts in the past, with files being formatted with different versions of black.

- id: black
language_version: python3.7
exclude_types: ['markdown', 'ini', 'toml']
- repo: local
Copy link
Contributor

Choose a reason for hiding this comment

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

could you explain a bit what the new changes are?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aahung aahung force-pushed the fix-black-version branch from c8d1987 to 76c0c38 Compare October 21, 2020 17:00
@aahung
Copy link
Contributor Author

aahung commented Oct 21, 2020

(just did a rebase on latest upstream/develop)

@aahung aahung requested review from sriram-mv and hoffa October 21, 2020 17:01
Copy link
Contributor

@hoffa hoffa left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Big improvement versus having versions scattered around!

@aahung aahung force-pushed the fix-black-version branch from 76c0c38 to 7c27ade Compare October 21, 2020 19:44
@aahung aahung force-pushed the fix-black-version branch from 5f763bc to 7d01ac8 Compare October 21, 2020 19:58
@aahung
Copy link
Contributor Author

aahung commented Oct 21, 2020

Just did a rebase and update the dev guide with a workaround how to configure black in IDE.

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.

5 participants