Skip to content

Paint it black #3502

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
Closed
bjlittle wants to merge 13 commits into from
Closed

Paint it black #3502

bjlittle wants to merge 13 commits into from

Conversation

@bjlittle
Copy link
Member

@bjlittle bjlittle commented Nov 1, 2019 •
edited
Loading

This PR enables black, the opinionated, uncompromising code formatter.

It's a good thing, and I've bought into the hype. Needless to say, so have many other leading scientific python packages, such as dask, xarray, and pandas. If you need more convincing,

... and it even has editor and IDE integration support 😀

This PR also incorporates support for pre-commit git hooks, which when enabled will allow the developer to ensure that all iris code in the repository is black compliant, by automatically executing black whenever a developer git commit command is performed. Note that, the pre-commit hooks run only against the code changed/new to the PR, and not over the whole iris repository - so it's not an expensive act.

In addition to enabling the black pre-commit git hook, other hooks I've deemed useful have also been enabled. For a list of supported hooks, see here.

Note that, black is also now part of the travis-ci workflow, to ensure that developers are actually committing black compliant code as part of their PR.

Also, for the first time ever, this PR make the entire code base flake8 compliant (with # noqa exceptions).

Still to do...

  • black format docs.
  • black format tests.
  • determine whether stickler-ci at black v18.9b0 conflicts with v19.10b0.
    • drop it, the stickler-ci version is too old 😞
  • add extra flake8 files to ignore for stickler-ci, to temporarily appease it.
  • decide whether to disable stickler-ci.
    • adopt flake8 pre-commit hook along withstickler-ci flake8 👍
  • disable flake8 in pre-commit (temporary).
  • make the code base flake8 compliant.
  • review the .flake8 files that we explicitlyexclude.
  • re-enable flake8 in pre-commit.
  • tidy/drop iris.tests.test_coding_standards.TestCodeFormat and iris.tests.test_coding_standards.StandardReportWithExclusions.
  • create issue to follow-up on skipped pp_load_rules unit test
  • update .github/CONTRIBUTING.md and/or developer notes to provide relevant pre-commit configuration details.
  • add black badge
  • rebase with master

@bjlittle bjlittle force-pushed the paint-it-black branch 5 times, most recently from d5cc8a2 to 2285d06 Compare November 1, 2019 11:59
stickler-ci
stickler-ci reviewed Nov 1, 2019
View reviewed changes
@bjlittle bjlittle force-pushed the paint-it-black branch 2 times, most recently from 60cfb52 to 01d7483 Compare November 1, 2019 14:04
stickler-ci
stickler-ci reviewed Nov 2, 2019
View reviewed changes
@bjlittle bjlittle force-pushed the paint-it-black branch 2 times, most recently from 4f335c7 to 9fea7e1 Compare November 3, 2019 03:49
@bjlittle bjlittle mentioned this pull request Nov 4, 2019
Closed
bjlittle
bjlittle commented Nov 4, 2019
View reviewed changes
lib/iris/tests/unit/fileformats/pp_load_rules/test__convert_time_coords.py
self.assertCoordsAndDimsListsMatch(coords_and_dims, expected)


class TestArrayInputWithLBTIM_0_3_1(TestField):
@unittest.skip("investigate failure")
def test_t1_scalar_t2_list(self):
Copy link
Member Author

@bjlittle bjlittle Nov 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

See #3508 as a promise to follow-up and investigate this failing unit test.

Discovered it thanks to enabling flake8 across the whole code base 😉

@bjlittle bjlittle added this to the v3.0.0 milestone Nov 6, 2019
@trexfeathers
Copy link
Contributor

trexfeathers commented Nov 7, 2019

@bjlittle I think it's important that we tick that final box regarding documentation before we make this change.

I appreciate there's a planned bigger effort to improve the developer docs but it would be wrong to wait until then to mention Black, although that may be the time to write something more detailed.

Thanks!

@bjlittle
Copy link
Member Author

bjlittle commented Nov 11, 2019

Replaced by #3518

It's easier to base these changes on a later version of master, rather than rebase/merge

@bjlittle bjlittle closed this Nov 11, 2019
@bjlittle bjlittle deleted the paint-it-black branch November 12, 2019 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@stickler-ci stickler-ci stickler-ci left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Status: Work in Progress Type: Enhancement Type: Infrastructure

Projects

None yet

Milestone

v3.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@bjlittle @trexfeathers @stickler-ci