Rename ncol parameter in legend to ncols #23198
Conversation
1b0219e to
7c3d4df
Compare
lib/matplotlib/legend.py
Outdated
| @@ -294,6 +294,7 @@ class Legend(Artist): | |||
| def __str__(self): | |||
| return "Legend" | |||
|
|
|||
| @_api.rename_parameter("3.6", "ncol", "ncols") | |||
There was a problem hiding this comment.
This is very old and pretty heavily used. I am not sure we should deprecate it? Can we just alias it and change the docs to the preferred grammar?
There was a problem hiding this comment.
Should I even make a release note then? Or just let (new) users slowly realize that ncols is the preferred name?
There was a problem hiding this comment.
I think a what's new is helpful....
b778f6d to
84882cc
Compare
There was a problem hiding this comment.
I think this is at the very border of being worth the trouble.
- The current usages of
ncolsare for subplots/gridspec, which is context-wise quite a bit away fromlegend(), so IMHO the inconsistency is not very obvious. ncolis too prominent for deprecation, which means we'll live forever with duplicate parameters.
If we do this, I suggest to make it explicit as laid out in the comments.
An option is of course to just close #12078 as won't fix. Should I do that instead? |
|
I'm actually in favour of this change. Sure they are semantically distinct, but having the exact same thing have different short forms is a nuisance ("loc"/"location" also comes to mind) because I can't remember which one belongs to which method. |
1349074 to
b9f9205
Compare
|
If you squash you need to keep the "co-authored by" comments in there. Feel free to ping me for a review if I forget in the next few days. |
|
Ahh, I used fixup, not squeeze... That probably explains it! (Now I realize the use case of squeeze. That always looked a bit useless to me and "covered" by rename and fixup, but clearly not in this case.) |
|
I'm not sure what fixup or squeeze are. I always just do an interactive rebase (rebase -i upstream/main). |
|
fixup and squeeze are the things you can write in front of commits when doing the interactive rebase. |
|
Ah squeeze=squash ;-) |
| @@ -333,6 +336,8 @@ def __init__( | |||
| frameon=None, # draw frame | |||
| handler_map=None, | |||
| title_fontproperties=None, # properties for the legend title | |||
| *, | |||
| ncol=1 # synonym for ncols (backward compatibility) | |||
There was a problem hiding this comment.
Can we do this? I think you have to leave ncol where it was for backcompat? We should prob move all of these to kwarg only...
There was a problem hiding this comment.
If someone uses this with a positional argument it will be picked up as ncols, so that should be fine. ncols will also come earlier when editors pop up the list of arguments, so I think this is really the way to go.
As #23166 is in, we are on the way to keyword only arguments.
PR Summary
Closes #12078
It seems like only legend uses ncol and that nrow is not used (only nrows).
Will add release note once I get a PR number and I see that I have not missed anything.Done!PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).