Renamed conversion interface mdims argument to groupby #1066
Conversation
| """ | ||
| if 'mdims' in kwargs: | ||
| if groupby: | ||
| raise ValuError('Cannot supply both mdims and groupby') |
philippjfr
force-pushed
the
deprecate_mdims
branch
from
fc0ecb3
to
b76703a
Compare
| else: | ||
| self._element.warning("mdims keyword has been renamed to " | ||
| "groupby and will be deprecated in " | ||
| "version 1.8.") |
There was a problem hiding this comment.
"will be deprecated" is ambiguous; isn't it now deprecated already? Plus it seems like we shouldn't promise a version 1.8 we might not have (in favor of 2.0). How about:
mdims keyword has been renamed to groupby; the name mdims is deprecated and will be removed after version 1.7.
|
Looks great; I think that will be much clearer. Also seems to add support for passing a single dimension, not just a list of dimensions. |
Yes, should have listed that explicitly, both the kdims and vdims arguments support this so it's been annoying me persistently that you had to pass a list here. |
|
Passing and ready to merge. |
| self._element.warning("mdims keyword has been renamed " | ||
| "to groupby; the name mdims is " | ||
| "deprecated and will be removed " | ||
| "after version 1.7.") |
There was a problem hiding this comment.
I never know whether it should be 'mdims' keyword ... or mdims keyword.... As we are referring to the name of the keyword, I think I would include the quotes (also ... renamed to 'groupby'). Anyway, this is a very minor issue and I don't mind if you prefer to leave it as it is.
There was a problem hiding this comment.
I agree the quote marks (or even backquote marks) would be the correct way, but figured we were being expedient.
|
Made one minor comment about the deprecation message but otherwise, I am happy to merge now. |
philippjfr
force-pushed
the
deprecate_mdims
branch
from
ff84751
to
15cdc5c
Compare
|
Comment addressed, ready to merge when tests pass. |
|
Looks good. Merging! |
As suggested in #1061 renaming mdims to groupby in the conversion interface
.tomethod is a lot clearer. This PR renames the kwarg while retaining support for the mdims argument along with a deprecation warning.