FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

DOC: Add note about locators at top of ticker docs by dopplershift · Pull Request #19002 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

DOC: Add note about locators at top of ticker docs - #19002

Merged
QuLogic merged 1 commit into
matplotlib:masterfrom
dopplershift:locator-note
Dec 17, 2020
ConversationCommits (1)ChecksFiles changed
Merged

DOC: Add note about locators at top of ticker docs#19002
QuLogic merged 1 commit into
matplotlib:masterfrom
dopplershift:locator-note

Conversation

dopplershift commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

PR Summary

The note about not sharing Locator instances should be more prominent than hiding in the base class docs, which most users have no need to examine (since they'll use one of many subclasses 99% of the time).

I opted not to add anything to the text_intro.py tutorial, but I'm open to doing so if someone has suggestions on the right level of detail for that.

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

anntzer commented Nov 25, 2020

Copy link
Copy Markdown
Contributor

See also #13482 and #13439.

jklymak commented Nov 25, 2020

Copy link
Copy Markdown
Member

As discussed in gitter, I think we should fix this properly. OTOH, happy for this to go in when the build passes....

timhoffm reviewed Nov 25, 2020
View reviewed changes
Comment thread lib/matplotlib/ticker.py Outdated
Comment thread lib/matplotlib/ticker.py Outdated
Comment thread lib/matplotlib/ticker.py Outdated

dopplershift commented Nov 25, 2020

Copy link
Copy Markdown
Contributor Author

If we want this in the 3.3 docs, then I'm happy to update. If we're actually going to fix properly for 3.4, then there's no point in putting this in.

jklymak commented Nov 25, 2020

Copy link
Copy Markdown
Member

I think it'd be good to get this in because I have a feeling we won't be able to re-engineer Locators and Formatters by 3.4 (though who knows! Where there is a will there is a way).

dopplershift force-pushed the locator-note branch 2 times, most recently from 3f1ad82 to 0497934 Compare November 26, 2020 21:20
DOC: Add note about locators at top of ticker docs
c1b0756 
The note about not sharing `Locator` instances should be more prominent
than hiding in the base class docs, which most users have no need to
examine.

dopplershift commented Nov 28, 2020

Copy link
Copy Markdown
Contributor Author

Should this be milestoned for 3.3-doc?

tacaswell added this to the v3.3-doc milestone Nov 28, 2020

dopplershift commented Dec 16, 2020

Copy link
Copy Markdown
Contributor Author

I don't think there's anything else this needs...

QuLogic commented Dec 17, 2020

Copy link
Copy Markdown
Member

Seems fine to me.

QuLogic merged commit e780592 into matplotlib:master Dec 17, 2020

lumberbot-app Bot commented Dec 17, 2020

Copy link
Copy Markdown

Something went wrong ... Please have a look at my logs.

It seem that the branch you are trying to backport to does not exists.

QuLogic commented Dec 17, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

@meeseeksdev backport to v3.3.3-doc

1 similar comment

QuLogic commented Dec 17, 2020

Copy link
Copy Markdown
Member

@meeseeksdev backport to v3.3.3-doc

meeseeksmachine mentioned this pull request Dec 17, 2020
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Dec 17, 2020
Backport PR matplotlib#19002: DOC: Add note about locators at top of …
dd88dfe 
…ticker docs
dopplershift deleted the locator-note branch December 17, 2020 06:34
timhoffm added a commit that referenced this pull request Dec 17, 2020
Merge pull request #19136 from meeseeksmachine/auto-backport-of-pr-19…
710ffd9 
…002-on-v3.3.3-doc

Backport PR #19002 on branch v3.3.3-doc (DOC: Add note about locators at top of ticker docs)

gribbg commented Dec 23, 2020

Copy link
Copy Markdown

Two other thoughts in this space:

  1. Can we update the documentation for set_major_locator and set_minor_locator to include Note: locator must be unique instance per axis?
  2. Any thoughts on making TickHelper.set_axis() issue a warning if self.axis is already set?

timhoffm commented Dec 23, 2020

Copy link
Copy Markdown
Member
  1. Basically 👍, though I would use the same wording as here.
  2. Generally 👍 on runtime warnings for non-intended usage. TickHelper is also the base class for Formatters, and AFAIK the restriction does not apply to them. So the warning would have to be implemented in the Locator class.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

QuLogic QuLogic approved these changes
timhoffm Awaiting requested review from timhoffm

Assignees

No one assigned

Labels

Documentation

Projects

None yet

Milestone

v3.3-doc

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL