Failure seems unrelated.

I am wondering if one should replace one of the legends (or possibly add one in the middle) that uses the rcParams?

Regarding the 0/1 issue, I'll add that to the agenda for the dev call tomorrow.

Oh, and if you know how to rebase and squash, please do that.

It would also make sense to edit the commit message and add the line (after two empty lines):

Co-authored-by: Tranquilled (or proper name)  <whatever the email is>

I don't know how to rebase and squash, but I can probably figure it out. And add the coauthor message too.

Are there any updates on the 0/1 issue that warrants changing the code here?

@tuncbkose we have instructions for squashing here:
https://matplotlib.org/devdocs/devel/development_workflow.html#rewriting-commit-history

Thank you for your work on this @tuncbkose however I'm going to push this to 3.8. There are a couple of outstanding questions that will take too long to sort out to sneak this in for 3.7.

• Should we make the input bool or dict as Shadow will take all of the Patch keyword arguments. As @timhoffm points out if we start making it possible to thread the color through it is likely there will soon be a demand for threading through the rest of the parameters. This then leads to the question of how to handle the rcparams as we do not currently have any structured values (there is no validate_dict) so either we would have to add that (which would be a lot of work) or accept that there are values you can pass at run time that can not be set in rcparams. Neither are great options.
• Another option would be to add a shadow_props kwarg. The upside of this is we side-step all of the rcparams issues (at least for a bit), but the major down side is now we have coupled kwargs (where the value of one kwarg affects if we even pay attention to the other). It would not be the first place we have this, but they are messy to document and use (but sometimes you need them for "Do What I Mean").
• @timhoffm and I need to sort out what to do about the {0, 1} cases (this is the second or third one to come up recently). On one hand I agree with Tim that the using 0 and 1 literals is place of True and False is not good, but on the other hand I am still very wary of ripping out the duck-typing (but not ideas I would endorse) with no warning.

While this seems like a very small change (and the actually implementation is straight forward), there are a bunch of follow-on consequences. Some of the more gnarly parts of the Matplotlib API are due to adding features that were relatively small but eventually began to interact in surprising ways!

I think my favorite path forward is:

• make the shadow kwarg to be None | bool | dict
• if setting the color via rcparams is a hard requirement add at legend.shadow_props.color entry (so we have space to add more if needed)

I think something like:

if shadow is None:
    self.shadow = rcparams['legend.shadow']
    self._shadow_props = {}
elif instance(shadow, dict):
    self._shadow_props = shadow
    self.shadow = bool(shadow)
else:
    self.shadow = bool(shadow)
    self._shadow_props = {}

# maybe
for k in ['color']:
    self._shadow_props.setdefault(k, rcparams[f'legend.shadow_props.{k}'])

...

if self.shadow:
    Shadow(self.legendPatch, 2, -2, **self._shadow_props).draw(renderer)

This:

• side-steps the "how much duck type preservation" by preserving the existing behavior at the cost of deciding the ambiguity of shadow={} should mean in terms of "bool like" not "props like"
• lets the user control the shadow color at the call sight
• also takes care of any future "but what about the " issues now
• gives an (optional) path to setting a subset of the properties via rcparams independently of setting if the shadow is shown by default or not

The biggest down side is a further growth of the number of rcparams, but I think that is marginally less bad than the existing rcparams getting more complex.

My last comment is the summary of a phone discussion with @QuLogic and @ksunden .

Moving this to draft, and marking inactive, however, @tuncbkose please let us know if you are still interested in this and we can move back to the queue...

I am happy to implement whatever that is decided to be preferable. It was (and still is) unclear to me if @tacaswell's last comments were only a suggestion or the agreed way to move forward. If it's the latter, I can probably make the necessary changes soon.

I think my favorite path forward is:

• make the shadow kwarg to be None | bool | dict

When we allow for this type variability in parameter values (which is quite reasonable), IMHO we can as well afford an intermediate color step: None | bool | color | dict. This allows us to do the color implementation now. The dict implementation can be left for later when the actual need for further configuration arises. But we know it's there as an extension path without the need to grow additional or conflicting further kwargs.

if setting the color via rcparams is a hard requirement add at legend.shadow_props.color entry (so we have space to add more if needed)

Is there some proven interest in rcParams availability? If not, let's start simple and leave this out - we have a number of setting that are not available through rcParams.

Rationale: If we start shadow configuration, this has to go into a new 'shadow_props' section immediately. As opposed to the input in parameters, rcParams can be read. While we could overload this with color (every color is truthy, so current (duck-typed) bool checks would continue to work), we cannot transparently change this to a dict. It would also be unwise to have rcParams['shadow'] == *a-color* and additionally start a rcParams['shadow_props'], due to inconsistent/duplicate handling of values.

I'm also considering a maybe too clever alternative: Can we turn the existing 'legend.shadow' into a namespace directly, i.e. 'legend.shadow.color' etc. while staying backward-compatible.

a) Parsing an rcParams file should be simple: promote 'legend.shadow' to 'legend.shadow.visible'
b) Code usage: We still need API compatibility. This would mean that rcParams would need to grow special getitem/setitem logic for 'legend.shadow' (at least for a transition period).

This is quite a bit of effort, but would allow a more canonical rcParams namespacing in the future. I think, we can pull this of, but it's only worth if there is really the need. Hence, the proposal to not touch rcParams for now (well, we have to explicitly raise on colors now; otherwise users can think colors are supported, but we actually duck-type them as bool).

Can we have a similar generalization strategy for rcParams?

• For now use legend.shadow valued None | bool | color
• If there is need for more, introduce legend.shadow_props

legend.shadow.visible
legend.shadow.color

Just from the peanut gallery...

This then leads to the question of how to handle the rcparams as we do not currently have any structured values (there is no validate_dict) so either we would have to add that (which would be a lot of work)

I do think that making some rcParams be dicts (with a specified key/value model) would be good. (See also #11231.)

Here is a first implementation of shadow as None|bool|dict without rcParam support (as a mixture of suggestions by @timhoffm and @tacaswell). Currently, the dict contents are not checked because I assume Patch will do that already. If requested I can re-add shadow=[color] as well, but in my opinion having to do {"color": [color]} is only slightly different without the risk of potentially deprecating it later.

One question I have in addition to general review: should rcParams give a specific error message saying dict is not currently supported?

Thanks for the update @tuncbkose. If you look at the code diff you can see there are some comments from bots. The flake8 comments need to be addressed, and should be straightforward. The CodeCov comments may or may not be sensible, but it’s worth considering whether the second one indicates a genuine gap in the tests.

I'm aware of the current formating/styling issues. I just didn't fix them yet, in case more changes are requested. If it would be more convenient for the reviewers, I'll happily fix the failing issues now.

Gentle ping @tuncbkose - please let us know if you need help addressing the reviews above. Cheers!

Thanks for the ping @melissawm. Sorry that I forgot about this, I should be much more attentive now, if anything further is needed.

These errors seem unrelated to me.

Do we want a set_shadow method to allow editing of the shadow properties post-hoc? currently the self.shadow is a bool and self._shadow_props is a private attribute, so no public way to update it, as far as I can tell.

I go with „not yet“. The signature is not quite obvious to me. Is it a single (dict | bool) parameter (possibly, but a bit awkward for set_shadow({„color“: „red“})) or can we unpack into separate args so that set_shadow(True) andset_shadow(color=„red“) - works? The latter may be difficult to write a clean signature for. Or we could expose the underlying artist for styling?

Overall, let‘s wait with this until a request for this is raised.

@ksunden it seems like your requested change is done.

Test failures appear to be due to qt problems unrelated to this PR, but rerunning just to be sure.

Congrats @tuncbkose on getting your first PR merged into Matplotlib - we hope to see you around for a second one! 😄