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

Style fixes. #15086

Merged
merged 1 commit into from Aug 22, 2019
Merged

Style fixes. #15086

merged 1 commit into from Aug 22, 2019

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Aug 19, 2019

... mostly found by pylint, plus some other stuff.
It did catch two tests in test_colorbar with the same name, one
shadowing the other :)


edit: also needed to update the tests that checked the exact error messages, so parametrized them at the same time.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@dopplershift
Copy link
Contributor

All the more reason to work to make sure we actually run 100% of our test code. 99.09% != 100% (though I guess it will go up with this PR.)

@anntzer anntzer force-pushed the style branch 3 times, most recently from 8b34965 to 0006a62 Compare August 20, 2019 10:32
@tacaswell tacaswell added this to the v3.2.0 milestone Aug 20, 2019
Copy link
Member

@WeatherGod WeatherGod left a comment

Choose a reason for hiding this comment

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

Everything else looks fine, just a couple small quibbles.

layout.
'''
"""
Make all elements in this tree none, signalling not to do any more layout.
Copy link
Member

Choose a reason for hiding this comment

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

Is it one 'l' or two for "signalling"? Both form look misspelled to me.

Copy link
Member

Choose a reason for hiding this comment

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

Also, this none here should probably be rendered as a literal None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"{0} instead of {1}.".format(x.shape, z.shape))

raise TypeError(
f"Shapes of x {x.shape} and z {z.shape} do not match")
Copy link
Member

Choose a reason for hiding this comment

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

"Shapes of x (2, 3) and z (2, 4) do not match" would look a bit odd.

I would go for Shapes of x and z do match: {x.shape} != {z.shape}.

Similarly in some other places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I think my formulation reads better than yours, but I'm happy to hear suggestions from others.

Copy link
Member

Choose a reason for hiding this comment

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

I think either is OK and tie goes to the incumbent.

lib/matplotlib/_layoutbox.py Show resolved Hide resolved
lib/matplotlib/_layoutbox.py Show resolved Hide resolved
lib/matplotlib/contour.py Outdated Show resolved Hide resolved
lib/matplotlib/contour.py Outdated Show resolved Hide resolved
lib/matplotlib/contour.py Show resolved Hide resolved
... mostly found by pylint.
It did catch two tests in test_colorbar with the same name, one
shadowing the other...
@tacaswell tacaswell merged commit 47c0764 into matplotlib:master Aug 22, 2019
@anntzer anntzer deleted the style branch August 22, 2019 23:12
@anntzer anntzer mentioned this pull request Jan 5, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants