Skip to content
This repository has been archived by the owner on Jul 8, 2021. It is now read-only.

Blackify #14

Merged
merged 19 commits into from Aug 13, 2020
Merged

Blackify #14

merged 19 commits into from Aug 13, 2020

Conversation

trexfeathers
Copy link
Contributor

@trexfeathers trexfeathers commented Aug 12, 2020

This PR adds Black and support for pre-commit git hooks, using SciTools/iris#3518 as a guide where appropriate.

Had to move conda_requirements.txt since Black was treating it differently when it was in the repo root, and I was unable to exclude it via a custom exclude entry in pyproject.toml - weird.

Black is currently returning errors for README.md and pyproject.toml, but this is the case for SciTools/iris as well and doesn't cause problems - we don't need these formatting and it seems simpler to not need a custom exclude entry at all.

This was referenced Aug 12, 2020
.flake8 Show resolved Hide resolved
.travis.yml Outdated
@@ -90,4 +90,4 @@ install:

script:

- pytest -v ./iris_ugrid/tests
- pytest -v --black ./iris_ugrid/tests
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems different to the way Iris is set up to test for black compliance
https://github.com/SciTools/iris/blob/d16f6767d4d775e2afd691de9c6514782ba2a90d/.travis.yml#L108-L113
In particular, it looks as though black --check is being run for iris. Is this test equivalent? If there is a difference, is there a reason for that difference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to leverage the extra elegance of pytest (which Iris doesn't use) by using the pytest-black plugin. Unfortunately, while pytest-black is under active development, it is not kept up to date on Anaconda, which has resulted in the deprecation error in Travis.

I am unsure whether the correct action should be to explicitly pip install pytest-black, or to invoke black the 'old-fashioned' way like Iris does. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... just trying the pip solution on Travis as a POC ...

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like the idea of using the latest features of pytest, since we are kind of using this repo to "test drive" pytest anyway. I guess there is a good case for making changes where pytest allows a more elegant solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK well that worked at least, and I've included a TODO to get rid of the pip line once conda gets a later pytest-black version.
Gonna take a break now. I await your verdict!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's probably worth noting that in Iris, black is pinned to version 19.10b0 https://github.com/SciTools/iris/blob/d16f6767d4d775e2afd691de9c6514782ba2a90d/requirements/test.txt#L4.
This approach means that Iris-ugrid will be tested against a different version of black than Iris. I don't think this should cause massive issues, but it might be worth thinking about.

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated
@@ -90,4 +90,8 @@ install:

script:

- >
echo $(black --version);
black --check $(INSTALL_DIR);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have not replicated Iris' line removing .gitignore - testing has shown that Black is successfully skipping .gitignore automatically.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Iris also has the line - export INSTALL_DIR=$(pwd) within the script section rather than the install section. I don't know if this is relevant or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's present here too, but further up - line 10

Copy link
Contributor

Choose a reason for hiding this comment

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

The current travis run seems to give "No Path provided. Nothing to do 😴" despite passing. I wonder if - export INSTALL_DIR=$(pwd) is only applying to the contex of the install: block of code and should be moved down to below line 91 where the script: section begins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed safer to declare it a second time. As you've alluded to, it seems that environment variables are independent between the code blocks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, "No Python files are present to be formatted. Nothing to do 😴" seems like an improvement at least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From local testing, I'm expecting:
All done! ✨ 🍰 ✨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mystery now is why I get 3 passes locally, but on Travis it doesn't like __init__.py...

Copy link
Contributor

Choose a reason for hiding this comment

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

From what I can tell, the only thing that needs changing now is a newline at the end of the __init__.py file, and I suppose it's good to know it's picking that up. I'd say this is good to go once that's done.

@trexfeathers trexfeathers marked this pull request as draft August 12, 2020 15:31
@trexfeathers trexfeathers marked this pull request as ready for review August 12, 2020 16:33
@trexfeathers
Copy link
Contributor Author

After some trial and error this is now ready for your review again @stephenworsley. Thanks for your patience.

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, I'm happy to merge this.

@stephenworsley stephenworsley merged commit 7d812b9 into SciTools-incubator:master Aug 13, 2020
@trexfeathers trexfeathers deleted the blackify branch August 13, 2020 08:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants