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

More cbook deprecations. #7999

Merged
merged 1 commit into from
Feb 1, 2017

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Jan 31, 2017

is_scalar_or_string was not used anymore, is_scalar was only used in one
place (in mplot3d) and I rewrote that snippet into something more robust.

@WeatherGod
Copy link
Member

The new logic isn't equivalent, but it is certainly a whole lot easier to understand. I am really not sure what edge cases the old logic was trying to catch here. A tenative +1 from me.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 31, 2017

That's why I wrote "more robust", not "equivalent" :-) The old version would get you the incorrect error message when passing incorrect x, y, z with different lengths, for example.

@dopplershift
Copy link
Contributor

Jeez, given how much cleaner the new version is, I'm happy so long as no tests break. Don't make it more convoluted until we have tests justifying it.

@@ -543,6 +543,7 @@ def file_requires_unicode(x):
return False


@deprecated('2.1')
def is_scalar(obj):
"""return true if *obj* is not string like and is not iterable"""
return not is_string_like(obj) and not iterable(obj)
Copy link
Member

Choose a reason for hiding this comment

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

is is_scaler supposed to be broader than is_numlike?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arbitrary objects (that are not iterable) are scalars, not numlike. Why?

Copy link
Member

Choose a reason for hiding this comment

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

'cause that confuses me 'cause aren't scalers numbers? What definition of scalers are we using then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't use this function and I exactly don't want to get bogged down into defining what its semantics should be.

# `_is_string_like` matches the behavior of 2D `plot` (via
# `_process_plot_var_args`).
if args and not is_string_like(args[0]):
zs = args[0]
Copy link
Member

Choose a reason for hiding this comment

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

why is zs = args.pop(0) a bad idea?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b/c there's no tuple.pop?

Copy link
Member

Choose a reason for hiding this comment

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

:( sorry got mixed up and thought args was a list.

@WeatherGod
Copy link
Member

yeah, the results of the tests will be what will convince me that this change is ok.

`is_scalar_or_string` was not used anymore, `is_scalar` was only used in one
place (in mplot3d) and I rewrote that snippet into something more robust.
@WeatherGod
Copy link
Member

ok, the tests pass. Is there a place in the docs where we are documenting deprecations? Or does that happen automatically with the decorator?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 1, 2017

I think we just grep for @deprecated("...").

@WeatherGod WeatherGod merged commit c158a02 into matplotlib:master Feb 1, 2017
@WeatherGod
Copy link
Member

Thanks!

@QuLogic QuLogic added this to the 2.1 (next point release) milestone Feb 1, 2017
@anntzer anntzer deleted the more-cbook-deprecations branch February 1, 2017 21:44
@QuLogic
Copy link
Member

QuLogic commented Feb 5, 2017

is_scalar_or_string is used by default by cbook.flatten and that is used in at least artist.py and container.py. It's causing deprecation warnings when building the docs, so it's not quite unused.

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.

5 participants