Skip to content

Deprecate two-args for cycler() and set_prop_cycle() #10879

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

Closed

Conversation

timhoffm
Copy link
Member

PR Summary

Following a discussion in #10662, this deprecates the two-arg forms

  • rcsetup.cycler(label, values)
  • Axes.set_prop_cycle(label, values)
    in favor of their keyword equivalents
  • rcsetup.cycler(label=values)
  • Axes.set_prop_cycle(label=values)

PR Checklist

  • Code is PEP 8 compliant
  • Documentation is sphinx and numpydoc compliant
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@WeatherGod
Copy link
Member

-1

@timhoffm timhoffm force-pushed the deprecate-set-prop-cycle-two-arg branch from 88e9907 to 7147c1a Compare March 26, 2018 02:02
@timhoffm
Copy link
Member Author

@WeatherGod Can you please elaborate on your -1?

@timhoffm timhoffm added this to the v3.0 milestone Mar 26, 2018
@WeatherGod
Copy link
Member

As discussed previously, the two-arg form is not broken. Also, the form comes from the cycler package and was the original form. Many users learn how to use cyclers from that package's documentation, not ours. If you feel strongly about deprecating a non-broken API design, then do it there first. Otherwise, you will just cause confusion.

In addition, do not change the unit tests! The feature has not been removed (indeed, you didn't even set a removal date). Therefore, it must continue to be supported until (if) it gets removed. I seriously question the need to do this.

The argument of "there must be only one way to do it" isn't a hard-and-fast rule. Look at the many ways that exists to create a dictionary in python. You can pass keyword arguments. You can pass a sequence of tuples. Dictionary comprehensions. As well as literal dictionary construction. Each form has its pros-and-cons, and I am glad they all exist. It gives me the flexibility to create a dictionary in a manner that fits well with the situation.

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.

Still a -1. But if this does go in, then at the very least, you need to set a deadline in the deprecation, and you need to wait on changing the unit tests until the 2-arg form is actually removed.

@timhoffm
Copy link
Member Author

I acknowledge you have a strong position on this as well. 😃

If you feel strongly about deprecating a non-broken API design, then do it there first. Otherwise, you will just cause confusion.

I don't think, it's possible to remove the two-arg form from the original cycler. There's exactly one reason for it. According to its docs:

This is useful for when the label cannot be a keyword argument (e.g., an integer or a name that has a space in it).

Removing would cut functionality. However, in matplotlib we don't have this use-case. Property names can always be keywords.

Where do we go from here?
I want new users to learn and use the kwarg API. It's more pythonic, and more flexible (works with one and multiple properties). I'm not dogmatic about deprecating the two-arg form if it's too difficult for a reason.

So, I propose the following compromise:

  • Don't deprecate anything.
  • Make the two-arg form only the third alternative instead of the second in the docstrings (This is already the case for the original cycler).
  • Rewrite the examples to use kwargs.
  • Leave the unit tests alone (though most of the tests are two-arg - probably for historic reasons. And testing one would be as good as the other. I'll leave it, hoping that nobody learns from the unit tests).

@WeatherGod
Copy link
Member

WeatherGod commented Mar 26, 2018 via email

@timhoffm
Copy link
Member Author

Will do a new PR tomorrow.

@timhoffm timhoffm closed this Mar 27, 2018
@timhoffm timhoffm deleted the deprecate-set-prop-cycle-two-arg branch June 10, 2022 21:16
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