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

fixed bug 6028 #6073

Merged
merged 4 commits into from Mar 6, 2016
Merged

fixed bug 6028 #6073

merged 4 commits into from Mar 6, 2016

Conversation

crazyo
Copy link
Contributor

@crazyo crazyo commented Feb 29, 2016

The problem was that hyphen-minus was converted to minus signs without differentiating those in math expressions and those in texts. I fixed it by adding an optional parameter nonMath so that hyphens in texts won't be converted.

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone Feb 29, 2016
@tacaswell
Copy link
Member

@mdboom @zblz

@mdboom
Copy link
Member

mdboom commented Feb 29, 2016

Thanks for investigating the issue.

I think actually, for the non-math context, get_unicode_index should always return ord(symbol) and do nothing else. The fact that it could fall through to return tex2uni[symbol.strip("\\")] is also a bug. So maybe add:

if non_math:
    return ord(symbol)

to the top and leave the rest of get_unicode_index as is.

Also, to be PEP8-compliant, the new keyword argument should be spelled non_math, not nonMath.

@crazyo
Copy link
Contributor Author

crazyo commented Feb 29, 2016

@mdboom Hi thank you for your response. I have made changes accordingly :-)

@mdboom
Copy link
Member

mdboom commented Feb 29, 2016

👍 from me, but I'll leave this up for a little while in case others have comments.

@zblz
Copy link
Member

zblz commented Mar 1, 2016

Looks good to me, even though the code might be clearer if a math flag instead of non_math was used. Double negatives like non_math=False make understanding the code a little bit more difficult.

@crazyo
Copy link
Contributor Author

crazyo commented Mar 1, 2016

@zblz I've made the changes :-)

tacaswell added a commit that referenced this pull request Mar 6, 2016
@tacaswell tacaswell merged commit 85089a1 into matplotlib:master Mar 6, 2016
@tacaswell
Copy link
Member

@crazyo Can you open a new PR to add a simple test for this?

tacaswell added a commit that referenced this pull request Mar 6, 2016
@tacaswell
Copy link
Member

backported to v2.x as ebfbc4a

@crazyo
Copy link
Contributor Author

crazyo commented Mar 7, 2016

yes, will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants