I am not super happy with the implementation :

_api.check_in_list(["all","row","col","none",0,1,False,True]

The error message is clearer but with the if conditions above,False,True, 0 and 1 are not actually values that can be passed tosharex andsharey. In this case their value is changed to'all' or'none'.

@vdbma, is the comment about the code re-organization from@timhoffm above clear?

Yes very clear, thanks for the feedback ! I will implement it when I have some time.

I implemented what you suggested, but there are still two points that bother me and to which I did not find an elegant solution (yet):

• As was discussed above_api.check_in_list is what formats the error message. However the values thatsharex takes when this function is called already has been normalised to one of"all", "row", "col", "none". Thus it will never occur thatsharex still has one of the values0, 1, False, True , yet we still check that it might. But we can't remove the "useless" values, because then the error message would not be clear regarding what values are allowed for the user to use. This feels somewhat inconsistent to me.
• With the current implementation (suggested by@timhoffm) passing any other integer/float value than 0 or 1 is not a problem. Do we actually want this behaviour ? (if so the test added intest_subplots.py::test_shared should be removed)

• As was discussed above_api.check_in_list is what formats the error message. However the values thatsharex takes when this function is called already has been normalised to one of"all", "row", "col", "none". Thus it will never occur thatsharex still has one of the values0, 1, False, True , yet we still check that it might. But we can't remove the "useless" values, because then the error message would not be clear regarding what values are allowed for the user to use. This feels somewhat inconsistent to me.

The fundamental issue here is that the logic needs special handling of some of the existing values. Thus, the_check_in_list() site does not see the full range of possible values. As long as we build the message from the check list, we either lack allowed values in the message or we have to extend the list beyond what can actually be used in the current context.

I'm mostly ok to have the extra values in here. - The alternative would be to expand the semantics of the_print_supported_values parameter so that we can do:

check_in_list(    ["all", "row", "col", "none"],    _print_supported_values=["all", "row", "col", "none", True, False],    sharex=sharex, sharey=sharey)

• With the current implementation (suggested by@timhoffm) passing any other integer/float value than 0 or 1 is not a problem. Do we actually want this behaviour ? (if so the test added intest_subplots.py::test_shared should be removed)

Yes. To be clear: Conceptually, we don't accept special values 0, 1 as synonyms to True and False. We only support the concept of duck-typing, which says that you can pass any truthy or falsy values. This is also what happens in most cases automatically when you have conditions likeif sharex:.

N.b.: For that reason, the check in list message shouldnot list 0, 1 explicitly.

Thanks you for the feedback ! I better understand the logic.

@vdbma are you still interested in working on this PR?

I'm going to push this to 3.8, but if it is done there is no reason it can not be merged for 3.7.

Yes, very much so ! Sorry for the late contribution.

As of now the documentation forsharex says:

...
sharex, sharey : bool or {'none', 'all', 'row', 'col'}, default: False
Controls sharing of properties among x (sharex) or y (sharey) axes:
True or 'all': x- or y-axis will be shared among all subplots.
False or 'none': each subplot x- or y-axis will be independent.
...

Should I change it so that it is explicit that non-str values will be cast to boolean or should it remain an implicit behaviour ?

Please leave it as is. The primary logic type here isbool. That many things can be interpreted as bool is a language property of Python, which is often used. It would be rather distracting to mention this in every context that supports it.

Yes, very much so ! Sorry for the late contribution.

Great and no worries!

This could do with a squash, by you if you're comfortable with it, or at merge time.

I've squashed during merge.

@vdbma thanks, and congratulations on your first contribution to Matplotlib! We hope to see you again.