Merged _bool from axis into cbook._string_to_bool #6686

Merged
merged 2 commits into from Jul 7, 2016

Conversation

Projects
None yet
6 participants
Member

Kojoley commented Jul 4, 2016

No description provided.

mdboom added the needs_review label Jul 4, 2016

@efiring efiring and 1 other commented on an outdated diff Jul 4, 2016

lib/matplotlib/cbook.py
@@ -784,14 +784,15 @@ def is_scalar_or_string(val):
return is_string_like(val) or not iterable(val)
-def _string_to_bool(s):
+def string_to_bool(s):
+ """Parses the string argument as a boolean"""
@efiring

efiring Jul 4, 2016

Owner

For this to be a general cbook function, I think it should accept case-insensitive "true" and "false" as well as "on" and "off".

@Kojoley

Kojoley Jul 4, 2016

Member

Yes, I thought about it. I will add them then.

tacaswell added this to the 2.1 (next point release) milestone Jul 5, 2016

Owner

tacaswell commented Jul 5, 2016

This makes the kwargs a bit more permissive in what they accept (which seems ok).

Why make the function public?

Member

Kojoley commented Jul 5, 2016

Any reasons why it should not? I think that private functions should not be used outside of the module while this function is used in multiple external places.

Owner

tacaswell commented Jul 5, 2016

The convention we use is that we can use private functions across modules within the project.

If it is public and we change it that is an API break,

Member

Kojoley commented Jul 5, 2016

I see no problem with making it public as it is quite simple and should not be changed frequently (or even at all). If my position is wrong then I will remake it private.

Member

WeatherGod commented Jul 5, 2016

You'd be surprise what changes. More precisely, you'd be surprised how many
people complain when old, unused functions are removed. Since this
function does not have anything to do with plotting, it would make sense to
keep it private to keep people from depending on matplotlib for
non-plotting use-cases.

On Tue, Jul 5, 2016 at 10:06 AM, Nikita Kniazev notifications@github.com
wrote:

I see no problem with making it public as it is quite simple and should
not be changed frequently (or even at all). If my position is wrong then I
will remake it private.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6686 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-PxEbTWvHdAxXv-0-q4X7Kh_IoHLks5qSmTwgaJpZM4JEeu2
.

Member

Kojoley commented Jul 5, 2016

Hmm. I see your point and it is strong, but the whole cbook is not about plotting and most functions it contains are public... why module itself is public then?

Member

WeatherGod commented Jul 5, 2016

We were young... we were foolish...

On Tue, Jul 5, 2016 at 10:58 AM, Nikita Kniazev notifications@github.com
wrote:

Hmm. I see your point and it is strong, but the whole cbook is not about
plotting and most functions it contains are public... why module itself is
public then?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#6686 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AARy-HttQ0OvDLoyhokR-_odq5Lzmo06ks5qSnEhgaJpZM4JEeu2
.

Kojoley added some commits Jul 4, 2016

@Kojoley Kojoley Merged `_bool` from axis into `cbook._string_to_bool`
* Modified `_AxesBase.grid` to respect updated behaviour
78e44a6
@Kojoley Kojoley Added support of 'true' and 'false' to `cbook._string_to_bool`
cfd6374
Owner

tacaswell commented Jul 5, 2016

Also, I suspect the existence of cbook pre-dates the convention that a leading underscore means 'private'.

Contributor

dopplershift commented Jul 5, 2016

Well, I think cbook also started as a landing site for things borrowed from places like: https://code.activestate.com/recipes/langs/python/

@tacaswell tacaswell merged commit 94f2fb8 into matplotlib:master Jul 7, 2016

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.007%) to 70.282%
Details

tacaswell removed the needs_review label Jul 7, 2016

Owner

tacaswell commented Jul 7, 2016

@Kojoley Thanks!

Kojoley deleted the Kojoley:move-string_to_bool branch Jul 12, 2016

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