CLN: timeseries in plotting clean up #28020
Conversation
charlesdong1991
changed the title
|
|
||
|
|
||
| class TestTimeDeltaConverter: | ||
| def setup_method(self, method): |
There was a problem hiding this comment.
We're trying to move away from the setup_-style of tests. Can you use the full name in the test?
| ) | ||
| def test_format_timedelta_ticks(self, x, decimal, format_expected): | ||
| tdc = converter.TimeSeries_TimedeltaFormatter | ||
| result = tdc.format_timedelta_ticks(x, pos=None, n_decimals=decimal) |
There was a problem hiding this comment.
would it be worthwhile to parametrize over pos values here? or would that obscure the logic we actually care about testing?
There was a problem hiding this comment.
pos is not being used in format_timdelta_ticks function, honestly i am not sure why/how this is used. So i just keep as is for now. @jbrockmendel
There was a problem hiding this comment.
Leaving it as is sounds fine, thanks for looking into it.
|
do you prefer to remove |
|
fancy a merge? :D @TomAugspurger |
|
Seems OK to me though not super familiar with this part of the code base. Looking at the comment preceding your change, is _format_coord something we can get rid of as well (OK as separate PR - just curious) |
|
lgtm. can you merge master and ping on green. |
|
ping @jreback |
|
Thanks @charlesdong1991 |
format_timedelta_ticksappears both intimeseries.pyandmatplotlib.converter.py, and there is no test. Then i look through the code base, it is only used inmatplotlib.converter.pyforTimeSeries_TimedeltaFormatter. So I remove the one intimeseries.pyand add test for it.black pandasgit diff upstream/master -u -- "*.py" | flake8 --diff