[Sprint] Rc fixes #2193

katyhuff · 2013-07-02T02:05:45Z

This pull request achieves approximately two things.

One is that it mostly fixes issue Can'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?).
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 :

type("astring") is str doesn't work for unicode, but isinstance("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.
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.
My test noticed that hinting should accept "True" as an option, so I added that to the validate hinting function.
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.
A few font styles were present in matplotlibrc.template that werent in the rcsetup defaults, so I added those.
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.
The test noticed a merged set of font names due to a missing comma (there is no such font as "ImpactWestern"!)
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 :

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.
I also made some changes to the spacing so that comments would line up nicer in one section.

New test files :

test_rcsetup.rc is a copy of the matplotlibrc.template .
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 )

…ts passed along with the file sometime.

…than the = that it currently uses.

…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.

NelleV · 2013-07-02T08:17:16Z

lib/matplotlib/rcsetup.py

        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)


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?

sure. will do.

katyhuff · 2013-07-02T13:01:24Z

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 · 2013-07-02T15:17:24Z

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 · 2013-07-02T15:27:09Z

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 keep matplotlibrc.template and the new rcparams.rc in sync. The difficulty of keeping rcsetup.py and matplotlibrc.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 of matplotlibrc.template to the uncommented version. Note that matplotlibrc.template is copied (verbatim) and installed to matplotlib/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 a StringIO object, and then pass that to mpl.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 · 2013-07-02T17:06:06Z

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.

katyhuff added 22 commits June 29, 2013 09:42

adds encoding as a function parameter

2a85769

when dealing with unicode strings, isinstance(u, basestring) will ret…

b300ef3

…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 )

Merge branch 'master' into unicode_rc

7eb7ec0

undid enccoding parameter. This can be added later if the encoding ge…

aede120

…ts passed along with the file sometime.

adds a test file and a template rc file for the rc_setup tests.

55475a0

default test passes.

8df2f0b

autopep8'd the test file.

55d4b44

The matplotlibrc template docstring.hardcopy must use a colon rather …

1cfedb9

…than the = that it currently uses.

because of commas, Impact and Western were being merged

09a2450

uncomments the matplotlibrc template

6d79027

errors arose for some (but not all) of these parameters inside apostr…

a41204d

…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.

The matplotlibrc should match the default, which is (f, ctrl+f)

3840623

template test passes.

1042544

got rid of print statements from debugging

be48e7d

unicode test runs

09bf2dd

merged unicode solution

cda758d

unicode test passes

1976db5

errant space.

7302c31

updates matplotlibrc.template to mat the defaults in the rcsetup module

febaa0d

Merge branch 'master' into rc_fixes

c3bfe6c

getting up to date with master

fa3f07d

Merge branch 'master' into rc_fixes

6bd355f

NelleV reviewed Jul 2, 2013
View reviewed changes

katyhuff added 2 commits July 2, 2013 07:49

makes a proper docstring and removes the assert

15b8b57

fixed typerror

4ce865c

katyhuff closed this Jul 2, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Sprint] Rc fixes #2193

[Sprint] Rc fixes #2193

katyhuff commented Jul 2, 2013

NelleV Jul 2, 2013

katyhuff Jul 2, 2013

katyhuff commented Jul 2, 2013

mdboom commented Jul 2, 2013

mdboom commented Jul 2, 2013

katyhuff commented Jul 2, 2013

[Sprint] Rc fixes #2193

[Sprint] Rc fixes #2193

Conversation

katyhuff commented Jul 2, 2013

NelleV Jul 2, 2013

Choose a reason for hiding this comment

katyhuff Jul 2, 2013

Choose a reason for hiding this comment

katyhuff commented Jul 2, 2013

mdboom commented Jul 2, 2013

mdboom commented Jul 2, 2013

katyhuff commented Jul 2, 2013