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.

Sign up for GitHub

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

Jump to bottom

Use Black code style #322

Merged
merged 29 commits into from
Oct 21, 2018
Merged

Use Black code style #322

merged 29 commits into from
Oct 21, 2018

Conversation

matthewfeickert
Copy link
Member

@matthewfeickert matthewfeickert commented Oct 18, 2018 •
edited
Loading

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 (diana-hep/pyhf@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
Copy link

coveralls commented Oct 18, 2018 •
edited
Loading

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 Compare October 18, 2018 17:11
@matthewfeickert matthewfeickert mentioned this pull request Oct 19, 2018
Open
@matthewfeickert
Copy link
Member Author

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
Add black as a developer dependency
40ad45d
@matthewfeickert
Add pyproject.toml to configure Black
4385111 
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.
@matthewfeickert
Apply Black to setup.py
f35da3c
@matthewfeickert
Guard against attempting to install Black in Python 2.7
d88aa34 
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.
@matthewfeickert
Apply Black to non-pyhf files
95581f2 
The result of running:

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

black validation/
@matthewfeickert
Specify greater than 3.6.0 to be safe
6f49e28
@matthewfeickert
Exclude the build directory from Black
0059267
@matthewfeickert
Add skip-numeric-underscore-normalization to Black defaults
5118e5a 
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/
@matthewfeickert
Apply Black to tests/
2868c80 
Result of running:

black tests/
@matthewfeickert
Set py36 to false in Black defaults
2f07c41 
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.
@matthewfeickert
Apply black to tests/ after rebase against master
4fa754c
@matthewfeickert
Apply Black to pyhf/modifiers/
8810837 
Result of running:

black pyhf/modifiers/
@matthewfeickert
Apply Black to pyhf/exceptions/
1d114b6 
Result of running:

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

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

black pyhf/optimize/
@matthewfeickert
Reapply Black after rebase
7814146
@matthewfeickert matthewfeickert force-pushed the feature/use-black-code-style branch from 9e13d1c to 4edba2d Compare October 21, 2018 19:23
@matthewfeickert matthewfeickert force-pushed the feature/use-black-code-style branch from 4edba2d to c03bc2c Compare October 21, 2018 19:29
@matthewfeickert
Copy link
Member Author

matthewfeickert commented Oct 21, 2018 •
edited
Loading

@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.

@matthewfeickert
For Black comapre against Python 3.6
3ee92dc 
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
@matthewfeickert
Have Black check also show the diff
9ad6fb2 
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
@matthewfeickert matthewfeickert deleted the feature/use-black-code-style branch October 21, 2018 23:02
@matthewfeickert matthewfeickert mentioned this pull request Jan 31, 2019
Open
@matthewfeickert matthewfeickert added the style Changes that do not affect the meaning of the code label Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukasheinrich lukasheinrich lukasheinrich approved these changes

@kratsg kratsg Awaiting requested review from kratsg

Assignees

@matthewfeickert matthewfeickert

Labels
style Changes that do not affect the meaning of the code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: code formatting with black
3 participants
@matthewfeickert @coveralls @lukasheinrich