Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Concise dates test #16215

Merged
merged 5 commits into from
Jan 16, 2020
Merged

Concise dates test #16215

merged 5 commits into from
Jan 16, 2020

Conversation

stevenjlm
Copy link

@stevenjlm stevenjlm commented Jan 14, 2020

PR Summary

PR fixes #15182. Two test cases added to test_dates.py both to test ConciseDateFormatter parameters formats and zero_formats respectively. Expected outputs are compared with generated outputs for different time intervals. The file was also formatted with autopep8, hence some minor edits elsewhere in the file--I can revert the formatting easily if requested.

PR Checklist

  • [ x] Has Pytest style unit tests
  • [ x] Code is Flake 8 compliant
  • [ N/A] New features are documented, with examples if plot related
  • [ N/A] Documentation is sphinx and numpydoc compliant
  • [ N/A] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [ N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

],
[datetime.timedelta(seconds=40),
['', '05', '10', '15', '20', '25', '30', '35', '40']
],
Copy link
Member

Choose a reason for hiding this comment

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

OK, these results are a little confusing, I suspect because you are starting 1 Jan? Does it make sense to start on a different date and a bit later in the day?

Copy link
Author

Choose a reason for hiding this comment

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

I made the last element in zero_formats "%S.%f' in stead of ".%f" to make the expected string '00' instead of the empty string above.

Copy link
Member

Choose a reason for hiding this comment

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

I think its a bit confusing to have the zero-format for seconds be seconds, but I guess this indeed tests the functionality.

'day: 22',
'03/1997',
'day: 22',
'04/1997',
Copy link
Member

Choose a reason for hiding this comment

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

I don't know that this should be laid out with so many carriage returns. I appreciate it may make it a bit easier to debug, but it makes it harder for other to scroll through the file.

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.

Overall looks like a good addition, plus or minus a couple fo comments

Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

This looks great. I have two minor comments.

return sts

d1 = datetime.datetime(1997, 1, 1)
results = ([datetime.timedelta(weeks=52 * 200),
Copy link
Member

Choose a reason for hiding this comment

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

The indentation here is strange. I'm suprised it's not being picked up by the pep8 checker. Can you check this is pep8 compatible?

ax.set_ylim(date1, date2)
fig.canvas.draw()
sts = []
for st in ax.get_yticklabels():
Copy link
Member

Choose a reason for hiding this comment

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

The following would probably be faster and more readable:

sts = [st.get_text() for st in ax.get_yticklabels()]

@tacaswell tacaswell added this to the v3.3.0 milestone Jan 15, 2020
[datetime.timedelta(weeks=52 * 200), [str(t) for t in range(1980,
2201, 20)]],
[datetime.timedelta(weeks=52), [
'1997', '02/1997', '03/1997', '04/1997', '05/1997', '06/1997',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
'1997', '02/1997', '03/1997', '04/1997', '05/1997', '06/1997',
'01/1997', '02/1997', '03/1997', '04/1997', '05/1997', '06/1997',

This bug is not failing the test. Why? Something is fishy here.

Copy link
Author

Choose a reason for hiding this comment

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

The output you suggest is what you get if you set,
formats = ['%m/%Y', '%m/%Y', 'day: %d', '%H hr %M min', '%H hr %M min', '%S.%f sec']
however, the original is indeed what you get with formats set the way it is in line 512.

Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks @stevenjlm !

@NelleV NelleV merged commit cc8b594 into matplotlib:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More tests ConciseDateFormatter needed
5 participants