Skip to content

fix leaked exception in RRuleLocator.tick_values #9025

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 4 commits into from
Aug 25, 2017
Merged

fix leaked exception in RRuleLocator.tick_values #9025

merged 4 commits into from
Aug 25, 2017

Conversation

gnaggnoyil
Copy link
Contributor

The tick_values method of RRuleLocator in matlotlib.dates module uses dateutil module to calculate the datetime padding in an axis and in case the padding go out of leftist bound it will be set to num2date(1.0). However, in the case when the calculation of datetime goes out of bound, not only the ValueError will be thrown, but also OverflowError will. Not catching OverflowError would cause this exception to propagate through the call stack and ultimately prevent the main program from executing. This patch is intended to fix the bug, and a case that would cause this bug is added in test_dates.py in test_RRuleLocator_dayrange function.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Aug 12, 2017
tacaswell
tacaswell reviewed Aug 12, 2017
View reviewed changes
lib/matplotlib/tests/test_dates.py Outdated
def test_RRuleLocator_dayrange():
ret = 0

try:
Copy link
Member

Choose a reason for hiding this comment

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

You do not need the try...except here. If it raises, then the test will fail (but the exception will be caught and the rest of the test suite will run).

Just a comment about how this is just testing 'does not raise' is enough.

@tacaswell
Copy link
Member

Thank you for your work on this!

The test failure is a style violation (just some trailing whitespace).


=================================== FAILURES ===================================
 PEP8-check(ignoring E111 E114 E115 E116 E121 E122 E123 E124 E125 E126 E127 E128 E129 E131 E226 E240 E241 E242 E243 E244 E245 E246 E247 E248 E249 E265 E266 E704 W503) 
[gw1] linux -- Python 3.6.2 /home/travis/build/matplotlib/matplotlib/venv/bin/python
/home/travis/build/matplotlib/matplotlib/lib/matplotlib/tests/test_dates.py:144:1: W293 blank line contains whitespace
^

I left a comment on the test, it can be simplified a bit.

In general 👍 .

If you have any questions please ask!

@gnaggnoyil
Copy link
Contributor Author

gnaggnoyil commented Aug 12, 2017 •
edited
Loading

@tacaswell Thanks for your reviewing! I've updated the pull request as requested. Does the travis-ci job for this project check every snapshot for a pull request, or only check for the latest one?

@dstansby dstansby merged commit fc4d690 into matplotlib:master Aug 25, 2017
@tacaswell
Copy link
Member

Thanks @gnaggnoyil ! I think this was your first Matplotlib PR, congratulations 🎉 Hopefully we will hear from you again!

To answer your question about travis, it triggers on every push to the PR branch an runs on the merge of the tip of the branch into the current master branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@QuLogic QuLogic QuLogic approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
topic: date handling
Projects
None yet
Milestone
v2.1
Development

Successfully merging this pull request may close these issues.
4 participants
@gnaggnoyil @tacaswell @QuLogic @dstansby