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

New style format str #2461

Merged
merged 4 commits into from
Dec 3, 2013
Merged

Conversation

tacaswell
Copy link
Member

Added a Formatter sub-class that can handle new-style format strings.

def test_formatstrformatter():
'''
This is slight overlap with test_axes.test_twin_axis_locaters_formatters, but
this is a much more through coverage of the format string
Copy link
Member

Choose a reason for hiding this comment

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

Typo. TBH, I'd ditch the docstring altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? I think we have a non-zero number of functionally overlapping tests. Given the talk I have see go by about trying to speed up the tests I assume eventually someone will try to par them down and documentation of where overlap is can't hurt. On the other hand, I found this via a pretty simple grep so....

Copy link
Member

Choose a reason for hiding this comment

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

This test's speed is pretty inconsequential in comparison to the visual tests we have - I just don't feel the docstring adds anything IMHO.

@pelson
Copy link
Member

pelson commented Sep 25, 2013

I'm in favour of doing this - the format method is the way to go with string formatting, and we should embrace it throughout. IMHO the test could be made simpler - as you say, there is no need to test Python's formatting capabilities, instead we just need to be certain the correct formatting is taking place, so split the test into two, and just check one case per class.

@pelson
Copy link
Member

pelson commented Sep 25, 2013

Good stuff @tacaswell.

@tacaswell
Copy link
Member Author

That test was a bit of me having fun building test structures for the sake of building test structures.

Any suggestion on what format string should be tested?

does the exact same thing as `FormatStrFormatter`, but for new-style
formatting strings. Also added `FormatStrFormatterOldStyle` as an
alias for `FormatStrFormatter` to be ready for that unknown day
when the printf-style string formatting is depreciated.
Copy link
Member

Choose a reason for hiding this comment

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

typo: depreciated -> deprecated

@tacaswell
Copy link
Member Author

I think I addressed everything but the issue of what to name the new class of if the existing one should a) pick up and alias and b) be deprecated

@WeatherGod
Copy link
Member

I will admit that I don't really know what to call these, so I will try
spending some time thinking about it and get back to you.

As for deprecation, I am -1. There is nothing inherently wrong with the
old-style approach, and many users are familiar and comfortable with it
(those with a C/C++ background and those from Matlab are quite familiar
with it). I really need a compelling reason to drop the old style
completely.

@tacaswell
Copy link
Member Author

Sorry, I was unclear about what I meant. I mean rename FmtStrFormatter to a name which is symmetric with what ever the new class is called.

@tacaswell
Copy link
Member Author

re-based, re-named and squashed

Added class `StrMethodFormatter` which is identical to `FormatStrFormatter`
only for new-style ('{}'.format) formatting methods.
@WeatherGod
Copy link
Member

I like the name. +1 from me.

@pelson
Copy link
Member

pelson commented Dec 2, 2013

I'd happily merge this once the PEP8 tests are resolved.
👍

@tacaswell
Copy link
Member Author

It seems the in-editor pep8 checker is apparently broken on the laptop.....

pelson added a commit that referenced this pull request Dec 3, 2013
Added a new style string Formatter.
@pelson pelson merged commit a528eaa into matplotlib:master Dec 3, 2013
@tacaswell tacaswell deleted the new_style_format_str branch December 3, 2013 16:13
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.

5 participants