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

Use Black code style #322

Merged
merged 29 commits into from Oct 21, 2018

Conversation

Projects
None yet
3 participants
@matthewfeickert
Copy link
Collaborator

commented Oct 18, 2018

Description

Let's go Code style: black

Resolves #185

This is very much a heads up WIP PR that I'm hoping that this will spark some conversation. I think that this is actually something that will be a nice benefit to us in the long run as it will make our commit diffs really only about the code, but we'll have to see everyone's thoughts.

Running Summary:

  • Add pyproject.toml to configure Black using --skip-string-normalization option to preserve single quotes
  • Apply Black to the entire code base
  • Add black --check . to CI to ensure that the code style is Black
  • Add Git pre-commit hooks to the developer environment and add it to the developer setup docs (c5224be)
  • Add Black code style badge to README

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR

Running proposed PR commit log:

  • Add black as a dependency for the development environment on installations with Python 3.6+
  • Add pyproject.toml to configure Black using --skip-string-normalization option to preserve single quotes
  • Apply Black to the entire code base
  • Add black --check --diff --verbose . to CI to ensure that the code style is Black
  • Add Git pre-commit hooks to the developer environment with pre-commit and add .pre-commit-config.yaml
  • Add pre-commit setup to the development environment setup docs
  • Add Black code style badge to README

@matthewfeickert matthewfeickert self-assigned this Oct 18, 2018

@coveralls

This comment has been minimized.

Copy link

commented Oct 18, 2018

Coverage Status

Coverage increased (+0.02%) to 97.734% when pulling 9ad6fb2 on feature/use-black-code-style into 0fdfcc3 on master.

@matthewfeickert matthewfeickert force-pushed the feature/use-black-code-style branch 2 times, most recently from c20f5fb to bb386f7 Oct 18, 2018

@matthewfeickert

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 19, 2018

I'm going to put this on pause until PR #285 is done, as this will need to get rebased off of that and it will be way easier to resolve conflicts here (default to take all of what is in origin/master in the rebase and then I can just apply Black to everything and verify that it is reasonable).

matthewfeickert added some commits Oct 18, 2018

Add pyproject.toml to configure Black
This is mostly following the example in the Black README, and sets the
line length to 88 (the default), and skips converting single quotes to
double quotes.
Guard against attempting to install Black in Python 2.7
Black is Python 3.6+ only. From the README:
> It requires Python 3.6.0+ to run but you can reformat Python 2 code
> with it, too.
Apply Black to non-pyhf files
The result of running:

black --exclude "pyhf/|tests/|validation/" .
Apply Black to validation/
Result of running:

black validation/
Add skip-numeric-underscore-normalization to Black defaults
As Python 2.7 is still supported and support for underscores in numeric
literals wasn't added until PEP 515 for Python 3.6+ then by default
Black must be run with the --skip-numeric-underscore-normalization
option (at least until 2020).

c.f.
- https://www.python.org/dev/peps/pep-0515/
- https://python3statement.org/
Apply Black to tests/
Result of running:

black tests/
Set py36 to false in Black defaults
As Python2 is still supported then the Python 3.6+ option, py36, must be
set to false in the default configuration as it will:
> will put trailing commas in function signatures and calls also after
> *args and **kwargs.
Apply Black to pyhf/modifiers/
Result of running:

black pyhf/modifiers/
Apply Black to pyhf/exceptions/
Result of running:

black pyhf/exceptions/
Apply Black to pyhf/tensor/
Result of running:

black pyhf/tensor/
Apply Black to pyhf/optimize/
Result of running:

black pyhf/optimize/

@matthewfeickert matthewfeickert force-pushed the feature/use-black-code-style branch from 9e13d1c to 4edba2d Oct 21, 2018

@matthewfeickert matthewfeickert force-pushed the feature/use-black-code-style branch from 4edba2d to c03bc2c Oct 21, 2018

matthewfeickert added some commits Oct 21, 2018

@matthewfeickert matthewfeickert requested review from kratsg and lukasheinrich Oct 21, 2018

@matthewfeickert

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 21, 2018

@lukasheinrich @kratsg There are still a few things to iron out to get the tests passing, but as this is changing a lot of files I thought I would ask if you have any thoughts on things you know you want changed now that megachannel PR is in (also huge yay for that!).

One thing to mention is that by moving all docstring comments into their places so that they are actually docstrings then doctest of course wants to test them. It doesn't seem like doctest knows how to test decorators (c.f. Issue #324) so for the time being this removes them from the docstring and turns them into comments (c.f. pyhf/events.py and pyhf/modifiers/__init__.py). This doesn't effectively change anything as they weren't even getting noticed by doctest before.

For Black comapre against Python 3.6
Only 1 version of the code needs to run it, so there is no harm in
setting it to only be run on Python 3.6 and not Python 3.6 or higher

@matthewfeickert matthewfeickert changed the title [WIP] Use Black code style Use Black code style Oct 21, 2018

@matthewfeickert matthewfeickert changed the title Use Black code style [WIP] Use Black code style Oct 21, 2018

@matthewfeickert matthewfeickert changed the title [WIP] Use Black code style Use Black code style Oct 21, 2018

Have Black check also show the diff
If Black detecs the need to format a file it will show the relevant diff

@lukasheinrich lukasheinrich merged commit f7d27c7 into master Oct 21, 2018

4 checks passed

WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 97.734%
Details

@matthewfeickert matthewfeickert deleted the feature/use-black-code-style branch Oct 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.