Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Sprint] Rc fixes#2193

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
katyhuff wants to merge24 commits intomatplotlib:masterfromkatyhuff:rc_fixes
Closed

Conversation

katyhuff
Copy link
Contributor

This pull request achieves approximately two things.

  1. One is that it mostly fixes issueCan't store Unicode values in .matplotlibrc #1713. It doesn't quite teach matplotlib to read in special encodings in the matplotlibrc (which would be lovely, but I didn't have time just now). However, it does teach matplotlib to treat unicode strings identically to bytestrings, (which I've been told may help with python 3 compatibility?).
  2. The second is that it creates some tests that query a few things about rcsetup. These tests including a test for the above unicode behavior and a test querying whether an uncommented version of the matplotlibrc.template currently provided actually validates and agrees with the default parameters in rcsetup (hint, it was close before, but now it's right on).

Changes in rcsetup.py :

  1. type("astring") is str doesn't work for unicode, butisinstance("astring", basestring) works for both bytestrings and unicode. (http://www.evanjones.ca/python-utf8.html). So, I exchanged instances of the former with the latter.
  2. My test found that sometimes the apostrophes weren't properly stripped from the matplotlibrc file and I was getting errors indicating that "'param'" did not equal "param" for various parameters. So, I made a validate_string function that strips those things too.
  3. My test noticed that hinting should accept "True" as an option, so I added that to the validate hinting function.
  4. The test found that deprecate_svg_paths doesn't actually return the variable after issuing the warning. The other deprecated functions do return the variable, so added that.
  5. A few font styles were present in matplotlibrc.template that werent in the rcsetup defaults, so I added those.
  6. Matplotlibrc.template uses full color names, but the defaults in rcsetup use the letter abbreviations. That's cool, but since they should probably be identical and the matplotlibrc.template file should use words for readability, I changed the rcsetup to use "black" instead of "k," for example.
  7. The test noticed a merged set of font names due to a missing comma (there is no such font as "ImpactWestern"!)
  8. Some of the defaults were provided as single strings or tuples, when they are really list objects (their validators return lists).

Changes in matplotlibrc.template :

  1. When in doubt about results from the tests, I mostly changed the matplotlibrc.template to conform to rcsetup.py. This included a few bugs, but mostly just slight adjustments to the expected parameter.
  2. I also made some changes to the spacing so that comments would line up nicer in one section.

New test files :

  1. test_rcsetup.rc is acopy of the matplotlibrc.template .
  2. test_rcsetup.py adds some tests, discussed above.

…urn True for either bytestrings or unicode strings. So, in order to allow unicode in the matplotlibrc, we need to use this typechecking rather than the former (which was type(s) is string )
…ophes. Removing the apostrophes seemed to work. I'm curious about why this occured for some but not all variables. Anyway, my test complained about these but not the others.
return [validate_color(c.strip()) for c in s.split(',')]
else:
assert type(s) in [list, tuple]
return [validate_color(c) for c in s]


def validate_string(s):
'return a clean string'
assert isinstance(s, basestring)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not a big fan of doing asserts in code. Not only do they raise exception that are hard to understand, but they are also only evaluated when the code is not run as optimized. Maybe we should do a proper verification, and raise a more explicit error message?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

sure. will do.

@katyhuff
Copy link
ContributorAuthor

The assert has been removed and the improper docstring is now proper, as requested by@NelleV . Thanks for the review!

As an aside, the rest of the class uses lots of improper docstrings and asserts. An issue should probably be added to clean it up, since I imagine a standard style is better than an inconsistent one. Maybe this fits within MEP8 goals - should we make a new issue?

@mdboom
Copy link
Member

This reminds me -- and it is probably best as a follow-on to this PR -- the matplotlibrc.template is a real mess formatting-wise, and it would be great to give in a consistent look, possibly with a little more whitespace throughout, to make it easier to use and read.

@mdboom
Copy link
Member

This is really cool work and fixes a lot of niggly inconsistencies, which I really like.

My one worry with this approach is that it might be hard to keepmatplotlibrc.template and the newrcparams.rc in sync. The difficulty of keepingrcsetup.py andmatplotlibrc.template in sync is already the source of many of the bugs you've found and addressed here.

So I wonder if there isn't some way we could automate the conversion ofmatplotlibrc.template to the uncommented version. Note thatmatplotlibrc.template is copied (verbatim) and installed tomatplotlib/mpl-data/matplotlibrc by the setup script, so we do have it available to use at testing time. The test could load it in, create a modified version in aStringIO object, and then pass that tompl.rc_context to read it. If we made it possible to distinguish between a line where the comment should be removed for testing and "real" comments, it might not be too bad. For example:

# This is a comment about something, because it has a space after the # character...#axes.foobar : 42

Here, you could only remove the comment character when it's followed by a non-space.

Am I making sense?

@katyhuff
Copy link
ContributorAuthor

Yep, I agree, you're making plenty of sense. Thanks!
That would be a good modification.
I thought about various ways of doing it in the build/install step , but I think putting it in the test step as you suggest makes more sense than any of those thoughts.
I'm happy to do that but it'll have to wait until later tonight.

I'll close this pr for now and reopen it when I'm done.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@katyhuff@mdboom@NelleV
[8]ページ先頭
©2009-2025 Movatter.jp