8000 Add tests for date module by oscargus · Pull Request #23013 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Add tests for date module #23013

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
Jun 5, 2022
Merged

Add tests for date module #23013

merged 1 commit into from
Jun 5, 2022

Conversation

oscargus
Copy link
Member
@oscargus oscargus commented May 8, 2022 •
edited
Loading

PR Summary

Extended date_ticker_factory to return SecondLocator and Microsecondlocator. The format strings can be discussed.

Removed dead code.

Added tests for more cases in dates.

Should we remove the special handling for dateutil <= 2.3? 2.4 appears to have been release 2014-2015 or so.

matplotlib/lib/matplotlib/dates.py

Lines 1526 to 1528 in 65e9620

# This fixes a bug in dateutil <= 2.3 which prevents the use of
# numpy arrays in (among other things) the bymonthday, byweekday
# and bymonth parameters.

PR Checklist

Tests and Styling

  • 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).
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).

8000

@anntzer
Copy link
Contributor
anntzer commented May 8, 2022

FWIW we already require dateutil>=2.7 (in setup.py).

@oscargus
Copy link
Member Author
oscargus commented May 8, 2022

Then the only problem remaining is that none of the special handling is actually tested, so hard to say if something breaks when removed...

jklymak
jklymak reviewed May 18, 2022
View reviewed changes
Copy link
Member
@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

I'm pretty confused by this. So far as I can tell we don't call date_ticker_factory in the code base. Am I missing some magic somewhere? Its always better to test the user facing interface, rather than the helper (which should probably be private). So does this actually do anything for the user? If so, what is the example?

@jklymak jklymak added the status: needs clarification Issues that need more information to resolve. label May 18, 2022
@oscargus
Copy link
Member Author

I was under the impression that it is used to return a suitable set of locator and formatter if one inputs a range in days. It is 17 years old, so I assume that someone uses it.

But you are right that it is not used in the code base nor is it in the documentation.

Personally, I stumbled upon it while trying to increase test coverage.

@oscargus
Copy link
Member Author
oscargus commented May 18, 2022 •
edited
Loading

At least someone uses it:

https://github.com/daq-tools/kotori/blob/d25565920429c17ca4b3b28f22d4d7b160405c0a/kotori/io/export/util.py#L61-L62
https://github.com/webclinic017/LiveBackTestingTrader/blob/8b4f5804c0aa6046128f6706582f9cde78a0519a/backtrader/backtrader/plot/formatters.py#L117-L120

@jklymak
Copy link
Member
8000 jklymak commented May 19, 2022

My vote would be to not improve it, and to deprecate it. It doesn't make sense for us to maintain a different ticker algorithm that we don't use or test, and if someone is using it, they can vendor.

@QuLogic
Copy link
Member
QuLogic commented May 19, 2022

date_ticker_factory was replaced by AutoDateFormatter/AutoDateLocator in ... 2006: 5f9468d

@oscargus
Copy link
Member Author

Sure, I can deprecate it then.

@oscargus oscargus force-pushed the datetests branch 2 times, most recently from 90d034c to e2a14ef Compare May 20, 2022 06:47
@oscargus oscargus changed the title Extend date_ticker_factory and add tests for date module Add tests for date module May 20, 2022
@oscargus
Copy link
Member Author

Removed the changes to date_ticker_factory here and will deprecate in another PR to keep it separate. Now: only added tests, dead code removal (at least I cannot trigger that part of the code and can probably argue for why it is not possible anymore), and corrected a comment.

@oscargus oscargus removed the status: needs clarification Issues that need more information to resolve. label May 20, 2022
@oscargus oscargus mentioned this pull request May 20, 2022
Merged
3 tasks
@greglucas
Copy link
Contributor

Needs a rebase, but otherwise looks good.

@oscargus
Copy link
Member Author
oscargus commented Jun 3, 2022

Rebased.

@oscargus oscargus marked this pull request as draft June 3, 2022 19:09
@oscargus
Copy link
Member Author
oscargus commented Jun 3, 2022

Gah! Rebased the wrong version... Will continue when on the computer with the latest version...

@oscargus
Copy link
Member Author
oscargus commented Jun 5, 2022

Rebased the correct version...

@oscargus oscargus marked this pull request as ready for review June 5, 2022 12:33
@greglucas greglucas merged commit f8ab952 into matplotlib:main Jun 5, 2022
@oscargus oscargus deleted the datetests branch June 5, 2022 13:55
@QuLogic QuLogic added this to the v3.6.0 milestone Jun 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

@greglucas greglucas greglucas approved these changes

Assignees
No one assigned
Labels
topic: date handling topic: testing
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@oscargus @anntzer @jklymak @QuLogic @greglucas
0