Skip to content

Deprecate parameter orientation of bar() and barh() #17322

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
wants to merge 1 commit into from

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented May 4, 2020

PR Summary

While stated to be internal, there's a parameter 'orientation' to bar(), which is also documented. To make this very clear, this PR deprecates 'orientation' and uses a private '_orientation' parameter instead.

The aim is to finally remove 'orientation' also from the docs, because it's rather confusing than helpful to document a parameter and say it's not to be used.

@timhoffm timhoffm added this to the v3.3.0 milestone May 4, 2020
@anntzer
Copy link
Contributor

anntzer commented May 4, 2020

Can't this be done by just using _rename_parameter on bar()? (the parameter is already keyword-only)

@timhoffm
Copy link
Member Author

timhoffm commented May 4, 2020

It's more a deprecation than a rename. And _rename_parameter does not allow to set a message. It would recommend using _orientation, which we certainly do not want.

@timhoffm timhoffm force-pushed the bar-orientation branch from 4c3cc9d to a40a9e9 Compare May 4, 2020 14:26
@anntzer
Copy link
Contributor

anntzer commented May 4, 2020

fair enough

@anntzer
Copy link
Contributor

anntzer commented May 4, 2020

I guess the question, though, remains as to why this should be private. For example we don't have hist() and histh(), just hist(..., orientation=...). It is true that the argument names are not optimal if using the "wrong" orientation, but I'm not sure that's a strong reason for the removal.
I guess I'm +/-0 on the removal :)

@timhoffm
Copy link
Member Author

timhoffm commented May 4, 2020

I guess the question, though, remains as to why this should be private.

My main motivation is that https://matplotlib.org/devdocs/api/_as_gen/matplotlib.axes.Axes.bar.html should not document orientation as "This is for internal use only." I don't want to have parameters, which we tell people to not use.

If we would design this from scatch, we could use direction-agnostic parameter names. But as is stands, the parameter of bar(..., orientation='horizontal') is rather confusing. Also, there's axh/vlines and axh/vspan. So there's other precedent of using two separate functions instead of orientation.

I think there should be one official way to draw horizontal bars. Other possibilites would be to deprecate barh() or to just remove the docs on orientation. But I think, hiding orientation is the least invasive change.

@anntzer
Copy link
Contributor

anntzer commented May 4, 2020

I think undocumenting it is good enough. (I'm not blocking the rename, just not so convinced by it.)

@@ -2392,7 +2392,14 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
error_kw.setdefault('ecolor', ecolor)
error_kw.setdefault('capsize', capsize)

orientation = kwargs.pop('orientation', 'vertical')
if 'orientation' in kwargs:
orientation = kwargs['orientation']
Copy link
Member

Choose a reason for hiding this comment

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

if we are going to rename we should add a check that we don't get both.

@tacaswell
Copy link
Member

I am 👍 on un-documenting it, 50/50 on renaming it.

It is already not in the signature and keyword-only (as we are doing kwargs popping) so the upside of renaming is relatively small (it is already hidden from auto-completers) and we can get it out of the docs by the text describing it from the docstring to a in-line comment. On the other hand, we would make some users change there code to remove some thing that has been documented more-or-less forever (looks like the doc and the functionality came in via f0be66e but was only labeled as "for internal use" in e078f91 for 2.1).

@tacaswell
Copy link
Member

Can we push this to 3.4?

@timhoffm
Copy link
Member Author

I've extracted the un-documentation part to #17504. We can decide for 3.4 if we want to deprecate the parameter in favor of a private version.

@timhoffm timhoffm modified the milestones: v3.3.0, v3.4.0 May 24, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 22, 2021
@jklymak jklymak marked this pull request as draft May 8, 2021 20:41
@timhoffm timhoffm modified the milestones: v3.5.0, v3.6.0 Jul 11, 2021
@timhoffm timhoffm removed this from the v3.6.0 milestone Apr 30, 2022
@timhoffm
Copy link
Member Author

timhoffm commented May 1, 2022

I'm not following up on this. The undocumented state is good enough.

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.

5 participants