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

Add rrulewrapper to docs#23048

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

Merged

jklymak merged 1 commit intomatplotlib:mainfromoscargus:rrulewrapperdocs

May 18, 2022

Merged

Add rrulewrapper to docs#23048

jklymak merged 1 commit intomatplotlib:mainfromoscargus:rrulewrapperdocs

May 18, 2022

Conversation

Copy link

Member

oscargus commentedMay 14, 2022•
edited
Loading

PR Summary

Closes#21880
Builds on#21993

PR Checklist

Tests and Styling

[N/A] Has pytest style unit tests (andpytest passes).
IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

oscargus added the Documentation label

May 14, 2022

Copy link

MemberAuthor

oscargus commentedMay 14, 2022

It works, although the documentation could be more verbose:

Copy link

MemberAuthor

oscargus commentedMay 14, 2022

Will improve the doc-strings and re-add links to rrulewrapper in the documentation.

oscargus marked this pull request as draft

May 14, 2022 14:45

oscargus force-pushed therrulewrapperdocs branch from1ddb753 to2915f1fCompare

May 15, 2022 13:23

oscargus marked this pull request as ready for review

May 15, 2022 13:25

timhoffm reviewed

May 15, 2022

View reviewed changes

lib/matplotlib/dates.py OutdatedShow resolvedHide resolved

oscargus force-pushed therrulewrapperdocs branch from2915f1f to843181bCompare

May 17, 2022 08:44

Add rrulewrapper to docs

3b53e9d

oscargus force-pushed therrulewrapperdocs branch from843181b to3b53e9dCompare

May 17, 2022 11:56

Copy link

MemberAuthor

oscargus commentedMay 17, 2022

rrule is gone andrrulewrapper updated.https://output.circle-artifacts.com/output/job/828a2bd9-86e5-471e-b738-011bcb043c3d/artifacts/0/doc/build/html/api/dates_api.html#matplotlib.dates.rrulewrapper

jklymak reviewed

May 17, 2022

View reviewed changes

lib/matplotlib/dates.py

		@@ -981,16 +981,28 @@ def __call__(self, x, pos=None):

		class rrulewrapper:
		"""
		A simple wrapper around a ``dateutil.rrule`` allowing flexible
		A simple wrapper around a `dateutil.rrule` allowing flexible
		date tick specifications.

Copy link

Member

jklymakMay 17, 2022

Choose a reason for hiding this comment

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

Can we add some clue here as to how this is supposed to be used? There are no documented methods on the class, which is probably fine, but as it stands this is kind of mysterious. Maybe:

Suggested change

	datetickspecifications.
	datetickspecifications.Usuallypasseduponinittoasubclassof`RRuleLocator`.

I will say that adding this wrapper seems a bit cumbersome versus just passingfreq,tzinfo etc to theRRuleLocator directly. If it were up to me, I'd deprecate rrulewrapper as public and get rid of it all together.