Skip to content

Conversation

@bjlittle
Copy link
Member

@bjlittle bjlittle commented Nov 11, 2019

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

@bjlittle bjlittle mentioned this pull request Nov 11, 2019
15 tasks
@bjlittle bjlittle added this to the v3.0.0 milestone Nov 11, 2019
.flake8 Outdated
#
.eggs,
build
compiled_krb
Copy link
Contributor

Choose a reason for hiding this comment

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

Should build and compiled_krb have commas after them?

Copy link
Member Author

Choose a reason for hiding this comment

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

@stephenworsley Nice, spot 👀

@bjlittle bjlittle force-pushed the paint-it-black-again branch from d215a2c to 366753a Compare November 11, 2019 12:45
delta_start = 24
delta_mid = 36
delta_end = 369 * 24
ref_offset = 10 * 24
Copy link
Contributor

Choose a reason for hiding this comment

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

These could be made comments.


patch = mock.patch("netCDF4.Dataset")
mock_netcdf_dataset = patch.start()
patch.start()
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps set to _.


Iris uses `black <https://black.readthedocs.io/en/stable/>`_ formatting and `flake8 <https://flake8.pycqa.org/en/stable/>`_
linting to enforce a consistent code format throughout the project. ``black`` and ``flake8`` can easily be installed
into your development environment with ``pip``::
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be worthwhile mentioning that this is also possible (and equivalent) with conda.

$ black .
$ flake8 .

Alternatively, we recommend using `pre-commit <https://pre-commit.com/>`_ to automatically run ``black`` and ``flake8``
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be worth bringing up that there is good information on IDE integration in the black documentation (and probably also the flake8 documentation, though I haven't looked so much at that myself).

Alternatively, ``black`` and ``flake8`` may be installed using ``conda`` instead. Note that, ``black`` also offers
`editor integration <https://black.readthedocs.io/en/stable/editor_integration.html#editor-integration>`_.

As a developer workflow, we instead recommend using `pre-commit <https://pre-commit.com/>`_ to automatically run ``black`` and ``flake8``
Copy link
Contributor

@stephenworsley stephenworsley Nov 11, 2019

Choose a reason for hiding this comment

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

"As a developer workflow," reads a bit clunkily. It almosts sounds as if you are a developer workflow offering a recomendation (consider how the structure of the sentence would change if it started with "as a developer,").

See also:
https://en.wikipedia.org/wiki/Garden-path_sentence

$ flake8 .

Alternatively, we recommend using `pre-commit <https://pre-commit.com/>`_ to automatically run ``black`` and ``flake8``
Alternatively, ``black`` and ``flake8`` may be installed using ``conda`` instead. Note that, ``black`` also offers
Copy link
Contributor

Choose a reason for hiding this comment

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

Having these two sentances on the same line seems to suggest they are talking about related ideas. To me, it seems as though editor integration is more closely related to the idea of workflows below.

Copy link
Contributor

@stephenworsley stephenworsley left a comment

Choose a reason for hiding this comment

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

Looks good now.

@stephenworsley stephenworsley merged commit ffcfad4 into SciTools:master Nov 12, 2019
@bjlittle bjlittle deleted the paint-it-black-again branch November 12, 2019 14:17
@pp-mo pp-mo mentioned this pull request Nov 15, 2019
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants