Skip to content

Unify set_pickradius argument #23196

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 4, 2022
Merged

Unify set_pickradius argument #23196

merged 1 commit into from
Aug 4, 2022

Conversation

oscargus
Copy link
Member

@oscargus oscargus commented Jun 3, 2022

PR Summary

Always store in self._pickradius. Added property where needed. Added check for positive number (not for Collection since a negative number can be used according to docs).

Not sure if the rename_parameter is really needed since these are typically not called using kwargs, but better safe than sorry.

PR Checklist

Tests and Styling

  • [N/A] Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@greglucas
Copy link
Contributor 

/home/circleci/project/lib/matplotlib/axis.py:docstring of matplotlib.axis.Axis:45: WARNING: py:obj reference target not found: pickradius

@oscargus
Copy link
Member Author

oscargus commented Jun 5, 2022 •
edited
Loading

I do not really understand the error. It seems like it picks up the docstring from get_pickradius method now, as opposed to earlier where it got the docstring from the constructor argument(?).

I made it a property rather than directly accessing the attribute (although there were still get_pickradius and set_pickradius). I could of course revert that part, but it seems somehow more correct to use a private attribute and a property (simply skipping the property could break stuff elsewhere I assume).

timhoffm
timhoffm reviewed Jun 6, 2022
View reviewed changes
@oscargus oscargus force-pushed the pickradiusunification branch 2 times, most recently from bedd6d5 to 0d847c2 Compare June 12, 2022 09:34
@tacaswell tacaswell added this to the v3.6.0 milestone Jun 24, 2022
@tacaswell
Copy link
Member

The docs failure looks real and needs to be fixed.

@timhoffm
Copy link
Member

axis_api.rst lists members explicitly by category. Adding the property Axis.pickradius at axis_api.rst l.153. Should fix the doc build.

@jklymak jklymak marked this pull request as draft June 30, 2022 10:52
@jklymak jklymak marked this pull request as draft June 30, 2022 10:52
@jklymak jklymak marked this pull request as draft June 30, 2022 10:52
@jklymak
Copy link
Member

jklymak commented Jun 30, 2022

Moved to draft pending fixing the doc build...

@oscargus oscargus force-pushed the pickradiusunification branch 2 times, most recently from 94e59a6 to 91f47d6 Compare August 4, 2022 06:34
@oscargus oscargus marked this pull request as ready for review August 4, 2022 07:58
@oscargus
Copy link
Member Author

oscargus commented Aug 4, 2022

I'm quite sure that the test failure is intermittent (lack of disk space?).

@greglucas greglucas merged commit 02e93e2 into matplotlib:main Aug 4, 2022
@oscargus oscargus deleted the pickradiusunification branch August 4, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timhoffm timhoffm timhoffm approved these changes

@greglucas greglucas greglucas approved these changes

Assignees
No one assigned
Labels
API: consistency Maintenance
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@oscargus @greglucas @tacaswell @timhoffm @jklymak