NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.4k

In contributing.rst, encourage kwonly args and minimizing public APIs.#14545

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

anntzer wants to merge1 commit intomatplotlib:masterfromanntzer:kwonly

Closed

In contributing.rst, encourage kwonly args and minimizing public APIs.#14545

anntzer wants to merge1 commit intomatplotlib:masterfromanntzer:kwonly

Conversation

Copy link

Contributor

anntzer commentedJun 13, 2019

... so that we can just point PR authors to it instead of explaining
again and again why.

Thought of this while looking at#14512.

Rewordings welcome.

PR Summary

PR Checklist

Has Pytest style unit tests
Code isFlake 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

In contributing.rst, encourage kwonly args and minimizing public APIs.

771ff62

... so that we can just point PR authors to it instead of explainingagain and again why.

anntzer added the Documentation label

Jun 13, 2019

WeatherGod reviewed

Jun 13, 2019

View reviewed changes

doc/devel/contributing.rst

		to add as little public API as possible. In particular, make sure that helper
		methods and internal attributes are private (i.e., start with an underscore).
		Remember that any non-private method can be directly called by the end user,
		and any non-private attribute can be directly set!

Copy link

Member

Choose a reason for hiding this comment

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

I'd probably leave out this sentence. It is extraneous and doesn't really explain why this is a "bad thing" (it can also confuse some contributors because they'll notice that they can do exactly the same thing with private methods and attributes).

Copy link

ContributorAuthor

anntzerJun 13, 2019

Choose a reason for hiding this comment

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

I'm not particularly in love with the wording here, but I do want to stress that point extremely strongly: if an attribute is public, the implementation should be robust against the end user modifying it; otherwise, make it private, or hide it behind a getter or a property.
If you have a better wording I'm all ears :)

Copy link

Member

tacaswellJun 13, 2019

Choose a reason for hiding this comment

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

Or add a "And therefore once we have released that API the behavior, arguments, and names can not be changed without going through a multi-version deprecation process." ?

tacaswell added this to thev3.2.0 milestone

Jun 13, 2019

timhoffm reviewed

Jun 13, 2019

View reviewed changes

Copy link

Member

timhoffm left a comment•
edited
Loading

Choose a reason for hiding this comment

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

Full agreement that we should document this better. (n.b. I'm planning to give a talk on API evolution at PyCon.DE).

You've said, you welcome rewording, so I wrote my own two paragraphs: 😄

API Changes

Changes to the public API must follow a standard deprecation procedure to prevent unexpected breaking of code that uses Matplotlib.

Deprecations must be announced via an entry indoc/api/next_api_changes.
Deprecations are targeted at the next point-release (i.e. 3.x.0).
The respective API must still be fully functional during the deprecation period. If possible, usage of that API should emit aMatplotlibDeprecationWarning (seecbook.warn_deprecated(), the@cbook.deprecated decorator and other helper functions inmatplotlib.cbook.deprecation that ease the deprecation of various aspects of an API).
Deprecated API may be removed two point-releases after they were deprecated.

Adding new API

Every new function, parameter and attribute that are not explicitly marked as private (i.e., start with an underscore) become part of Matplotlib's public API. As discussed above, changing the existing API is cumbersome. Therefore, take particular care when adding new API:

Mark helper functions and internal attributes as private by prefixing them with an underscore.
Carefully think about good names for your functions and variables.
Try to adopt patterns and naming conventions from existing parts of the Matplotlib API.
Consider making as many arguments keyword-only as possible.
.. seealso::API Evolution the Right Way -- Add Parameters Compatibly__

__https://emptysqua.re/blog/api-evolution-the-right-way/#adding-parameters

Copy link

ContributorAuthor

anntzer commentedJun 13, 2019

It's great :p Do you want to PR it separately, or should I just copy-paste that?

Copy link

ContributorAuthor

anntzer commentedJun 13, 2019

Although I do want to comment on

The respective API must still be fully functional during the deprecation period.

I think my view is closer to (with pretty bad wording)

The deprecated API should, to the maximum extent possible, remain fully functional during the deprecation period. In cases where this is not possible, the deprecation must never make a given piece of code do something different than it was before; at least an exception should be raised.

(Basically, if something changes and the user gets an exception, the user knows that, well, something needs to be fixed, or at least they'll pin to the previous release. If the behavior of the code changes, on the other hand, the user may well generate incorrect plots without ever realizing it.)

See e.g.#14544...