Skip to content

Patch for issue #6009 #6014

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

Merged
merged 7 commits into from
Jun 6, 2016

Conversation

afvincent
Copy link
Contributor

Here is a patch for issue #6009 about the EngFormatter in matplotlib.ticker.

It simply enforces a space when there is no SI prefix but yet a unit symbol. For example “10 seconds” should be formatted as “10 s” by the EngFormatter (previously “10s”).

@WeatherGod
Copy link
Member

Should add a unit test to exercise this. Preferably not an image test (we have plenty of those), but one that tests the strings coming out of EngFormatter.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Feb 18, 2016
@@ -960,7 +960,11 @@ def format_eng(self, num):

formatted = format_str % (mant, prefix)

return formatted.strip()
formatted = formatted.strip()
if (self.unit is not "") and (prefix is self.ENG_PREFIXES[0]):
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 this should be == not is. It is just a little less brittle.

@tacaswell
Copy link
Member

Left minor comment, but other wise 👍

@afvincent
Copy link
Contributor Author

I don't understand why my last commit broke the Travis CI build test: it seems to me that the changes are really minor, aren't they? Furthermore, when looking at the failure logs in “Details” section of Travis, one can find errors like FAIL: matplotlib.tests.test_colorbar.test_colorbar_closed_patch.test and all the 6 (unexpected) failures relate to test_colorbar. How can it be impacted by a change in ticker.EngFormatter?

@jenshnielsen
Copy link
Member

Travis is broken because the merge of another pr broke master is is not caused by your pr

@afvincent
Copy link
Contributor Author

Ok, thank you for the explanation :).

@tacaswell
Copy link
Member

Can you rebase so we can merge it?

@afvincent
Copy link
Contributor Author

I think I've done a mistake on this branch with my git local repo (what a change…). I used the same branch to implement the enhancement I presented in #6533 (which also fix this issue btw). Should I go back in time with git and then rebase, or do something else? I guess opening a new PR (on a branch from an up-to-date master) which fixes this small issue and also implements the space_sep new option isn't really a clean option, is it?

afvincent added 6 commits June 5, 2016 15:34
Hope it is what was expected when asking for some unit (non image) test.
Sorry, I was using a different name than “mticker” to import the ticker module, and I had forgotten to change it when committing the new unit test test_EngFormatter_formatting()…
Now “All right” on pep8online.com for the method format_eng in the class EngFormatter.
@afvincent afvincent force-pushed the afvincent-patch-issue-6009 branch from 5161bfa to 6d584ad Compare June 5, 2016 13:39
@afvincent
Copy link
Contributor Author

I rebased after deleting my last commit (the one with the enhancement #6533). Hopefully it will be OK.

@tacaswell tacaswell merged commit 0ac187d into matplotlib:master Jun 6, 2016
tacaswell added a commit that referenced this pull request Jun 6, 2016
FIX: EngFormatter without but without prefix

closes #6009
@tacaswell
Copy link
Member

tacaswell commented Jun 6, 2016

backported to v2.x as 59e0ab9

@QuLogic QuLogic modified the milestones: 2.0 (style change major release), 2.1 (next point release) Jun 6, 2016
@afvincent afvincent deleted the afvincent-patch-issue-6009 branch June 6, 2016 16:40
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.

6 participants