Skip to content
This repository has been archived by the owner on Dec 21, 2023. It is now read-only.

add pre-commit and black support #2956

Merged
merged 39 commits into from
Feb 4, 2020

Conversation

guihao-liang
Copy link
Collaborator

downstream of #2943

(tc58) ➜  gui-1-36 git:(01-24-pre-commit) ✗ git commit -m "add pre-commit and black support"
Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check Yaml...............................................................Passed
Check for added large files..............................................Passed
black................................................(no files to check)Skipped
[01-24-pre-commit c94fdd374] add pre-commit and black support
 3 files changed, 50 insertions(+)
 create mode 100644 .pre-commit-config.yaml
 create mode 100644 pyproject.toml

When I do this commit, it performs a bunch of correctness checks for me. The setup is one-time thing.

@guihao-liang
Copy link
Collaborator Author

Trim Trailing Whitespace.................................................Passed
Fix End of Files.........................................................Passed
Check Yaml...........................................(no files to check)Skipped
Check for added large files..............................................Passed
black................................................(no files to check)Skipped
[01-24-pre-commit da42b5367] install pre-commit hooks when ./configure
 2 files changed, 4 insertions(+), 7 deletions(-)

You will love this tool.

@guihao-liang guihao-liang requested a review from nickjong January 24, 2020 22:21
@znation
Copy link
Contributor

znation commented Jan 25, 2020

Does it block committing if a check fails? Is it easy to opt out of this behavior (either in general, or for a specific commit)?

@guihao-liang
Copy link
Collaborator Author

Does it block committing if a check fails? Is it easy to opt out of this behavior (either in general, or for a specific commit)?

If the check fails, it won't let you commit. You can skip the check by using

# SKIP=<check_id> git commit -m "blah"
SKIP=check_yaml git commit -m "blah" # will force to skip yaml check

Yes! It's easy to opt-out. I can add script to opt-out. But for us, I think it's better everybody opts in.

@guihao-liang
Copy link
Collaborator Author

@znation @TobyRoseman, I provide an option for the user to opt-out. Just let me know if you have more questions.

@guihao-liang
Copy link
Collaborator Author

guihao-liang commented Feb 1, 2020

Adding per-commit introduces a failure in pytest, #2972, which is resolved by #2973. Ready to merge.

@guihao-liang
Copy link
Collaborator Author

Passed all relevant tests. Ready to merge hand in hand with #2943.


[tool.black]
line-length = 88
target-version = ['py27', 'py35', 'py36', 'py37', 'py38']
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't support Python 3.8. So you probably don't want that here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's right! I will delete it.

@@ -35,24 +34,32 @@ function linux_patch_sigfpe_handler {
fi
}

$PIP install --upgrade "pip>=8.1"
$PIP install --upgrade "pip"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you removed the >=8.1?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think pip 8.1 is really old now. We don't have to hardcode it. The current pip is already 20.0.2. >=8.1 is redundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, you have the --upgrade so that should work.

@@ -35,24 +34,32 @@ function linux_patch_sigfpe_handler {
fi
}

$PIP install --upgrade "pip>=8.1"
$PIP install --upgrade "pip"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, you have the --upgrade so that should work.

@guihao-liang guihao-liang merged commit 96108b6 into apple:master Feb 4, 2020
@guihao-liang guihao-liang deleted the 01-24-pre-commit branch February 4, 2020 03:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants