Well Travis is really angry at me for the b'dotted' test case ^^! (Too bad, the tests pass locally on my computer with Python 2.7…) I am going to investigate this.

Ok, it looks like Python 2 (, which I am working with) is less strict about string-like object comparisons:

# Python 2.7.12 |Continuum Analytics, Inc.| (default, Jul  2 2016, 17:42:40) 

In [1]: b'dotted' in [u'dotted']
Out[1]: True

In [2]: 'dotted'.encode('ascii') in [u'dotted']
Out[2]: True

than Python 3:

# Python 3.6.0 |Continuum Analytics, Inc.| (default, Dec 23 2016, 12:22:00) 

In [1]: b'dotted' in ['dotted']
Out[1]: False

In [2]: b'dotted'.decode() in ['dotted']
Out[2]: True

In [3]: 'dotted'.encode('ascii').decode() in ['dotted']
Out[3]: True

The commit 973dd5e should fix the previous failures, by using the decode method if it is available.

Hum, Travis seems a bit happier with Python 3, but I just tried locally _validate_linestyle('dotted'.encode('utf-16') on Python 3, and now I understand better @tacaswell's comment in #8145 😞… It raises a nice UnicodeDecodeError as the enconding is not UTF-8.

What is the usual way to deal with this kind of encoding problem?

PS: I'll take take of the PEP8 issue, do not worry.

Ok, I had a look at how matplotlib.text.Text handles the problem of encoding. It accepts “string or anything printable with '%s' conversion”. So actually, I wonder if it would not be better to do something similar in _validate_linestyle if we want to avoid avoid raising a UnicodeDecodeError exception when _validate_linestyle is given a “string” with an “exotic” encoding. For example

    try:
        return _validate_named_linestyle("{:s}".format(ls))
    except KeyError:
        # ls is not a valid name of line style.
        pass

should avoid raising a UnicodeDecodeError with UTF-16 & Co. Or would it be better to really raise a UnicodeDecodeErrorexception? In this case,

    try:
        return _validate_named_linestyle(six.text_type(ls))
    except KeyError:
        # ls is not a valid name of line style.
        pass

may be more consistent with the other string checking done in rcsetup rather than trying to call the decode method if it is available (like in 973dd5e).

Removing the need_review flag for the moment because handling encoding has appear to be a bit harder than I expected and the proper way to do it has still to be chosen.

Just pass the user input through, in 3, users passing bytes in is wrong and should fail, on 2 it happens to work due to the warts of string v unicode on python2.

Ok thanks for the comment @tacaswell . I'll do this in a moment.

Note to myself: go back to before the decode commit, and play with six.PY3 in the test.

Rebased! I squashed some similar commits and dropped the former one that attempted to decode the arguments.

@tacaswell No more attempt to decode the arguments, and I modified the tests to take care of the difference between Python 2 and Python 3. TBH, I struggled a bit to find a clean way to do the latter: I hope the style of the new version of the tests is fine. On my local computer with Python 2, the tests are passing (and the doc is building): hopefully Travis with Python 3 will agree.

The new approach may not play nicely (as in, cause a warning) due to the same issue as #7658? I haven't checked, but you may want to have a look.

@anntzer Thanks, I'll have a look this weekend.

Based on @anntzer's remark, it appears that indeed

np.array([1, 2]) in ['dotted', 'solid']

raises a FutureWarningbecause one day the comparison that is done under the hood by in will be element-wise (the warning will then become a ValueError exception, which does not belong to the ones that are currently captured in this PR). Actually, in the precise case of this PR, this FutureWarning is not raised because we ignore the case, which calls .lower() before any in operation, and thus raises an AttributeError error that is correctly caught if a Numpy array is given as on-off sequence.

With 97234b5, I reintroduce some early type checking to avoid any comparison of a Numpy array with strings. I agree it is kind of a nuke for a problem that does not even currently exist, but some (super) thin reasons to support this may be:

• it makes the exception catching clearer (one can remove the AttributeError that may be raised by .lower())
• if one day one want not to ignore the case of the argument, it will still work properly… (eventhough this API break is not very likely to happen ;) )
• well, it works as well as the other solution (at least on Python 2 the tests pass locally, I hope Travis will agree on Python 3 ^^).

@tacaswell Ping if you want to have a look when Travis will have run (TL; DR: decode is no more 🐑)

@anntzer I hope you will notice the effort of not using is_string_like, despite the urge of doing so on first thought 😁. (ref: #8011)

@afvincent Congratulations, you've earned un bon point. (Dix points = une image. Dropping Python2 support = many many points :-))

@anntzer You know, I had a few teachers that were doing so in primary school 😄.

Abouy dropping Python2, well… Let's say that I made a “mistake” when I started to learn Python at the beginning of my Ph.D. But I promise, I'll switch for my next big research project (2020 is coming close anyway…). We'll see if the bounty points lasts until then ;). If it doesn't, at least I'll have access to all the wonderful things like @, f-strings & Co!

You know, I grew up in France... (in fact, pretty close to where you did your PhD...)

Rahh, Travis does not seem to be happy (@anntzer you may want to take your bon point back). I think I understand why (if I am right, I was lucky about exceptions catching before…): I'll push a commit when I will have a fix.

Rebased (and squashed) with a new version! The former spirit is still there but now the exception catching should be more robust, both on Python 2 (a local pytest runs smoothly with test_rcparams.py and the docs are building) and on Python 3 (manual testing in an interactive session, I hope Travis will agree with me this time).

For the record, previously on both Python versions, the exception handling was flaky or error-prone. For example on Python 2, a UnicodeDecodeError was not caught with arguments like 'dotted'.encode('utf-16'), causing a short exit that was not expected. Unfortunately, this exception was then caught by pytest, which was expecting a ValueError and was thus marking the test as fine… On Python 3, there was (at least) a funny doubly wrong behavior: byte-like ls argument that were of even-length were passed to the instance of validate_nseq_float, which was converting them into funny on-off ink sequences (for example validate_nseq_float()('dotted'.encode('utf-16')) corresponds to [255.0, 254.0, 100.0, 0.0, 111.0, 0.0, 116.0, 0.0, 116.0, 0.0, 101.0, 0.0, 100.0, 0.0] 😄 )

Ok, now Travis stays calm :)! Ping to @tacaswell if you want to have a look, now that Travis is peacefull. And @anntzer because there have been quite a few changes since you had a look yesterday (sorry for that 🐑 ).

Re-rebased: squashed to 2 single commits and reordered them (1. code changes, 2. update of the related test).

The only difference with the previous commits should be:

• hasattr(ls, 'decode') <- isinstance(ls, (bytes, bytearray)), because “Explicit is better than implicit;”
• more useful exception messages for debugging by adding the “representation” of the ls argument (done with "Yada yada {!r} yada".format(ls), because 2020 is far away and I like format :) ).

This looks good to me.

Thanks!

Thanks to all who reviewed the PR :). It was a windy journey, but an instructive one from my side ^^.

You have hit on both why the bytes / unicode changes were made in python3 and why the transition has been a bit rough 😈 .

There does not appear to be a _validate_linestyle in v2.0.x; @afvincent can you verify if this needs backporting?

@QuLogic Indeed, backporting does not seem required. #8040 (which is fixed by this PR) was not backported but milestoned for 2.1 (by @tacaswell) instead: that is why there is no _validate_linestyle in v2.0.x branch ⇒ removing the need_backport flag.