New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Renamed conversion interface mdims argument to groupby #1066

Merged
merged 3 commits into from Jan 17, 2017

Conversation

Projects
None yet
3 participants
@philippjfr
Member

philippjfr commented Jan 16, 2017

As suggested in #1061 renaming mdims to groupby in the conversion interface .to method is a lot clearer. This PR renames the kwarg while retaining support for the mdims argument along with a deprecation warning.

"""
if 'mdims' in kwargs:
if groupby:
raise ValuError('Cannot supply both mdims and groupby')

This comment has been minimized.

@jbednar

jbednar Jan 16, 2017

Member

ValueError?

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

Thanks.

else:
self._element.warning("mdims keyword has been renamed to "
"groupby and will be deprecated in "
"version 1.8.")

This comment has been minimized.

@jbednar

jbednar Jan 16, 2017

Member

"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.

This comment has been minimized.

@philippjfr

philippjfr Jan 16, 2017

Member

Agreed.

@jbednar

This comment has been minimized.

Member

jbednar commented Jan 16, 2017

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.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

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.

@philippjfr philippjfr requested a review from jlstevens Jan 16, 2017

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 16, 2017

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.")

This comment has been minimized.

@jlstevens

jlstevens Jan 16, 2017

Member

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.

This comment has been minimized.

@jbednar

jbednar Jan 16, 2017

Member

I agree the quote marks (or even backquote marks) would be the correct way, but figured we were being expedient.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 16, 2017

Made one minor comment about the deprecation message but otherwise, I am happy to merge now.

@philippjfr

This comment has been minimized.

Member

philippjfr commented Jan 17, 2017

Comment addressed, ready to merge when tests pass.

@jlstevens

This comment has been minimized.

Member

jlstevens commented Jan 17, 2017

Looks good. Merging!

@jlstevens jlstevens merged commit 52f219c into master Jan 17, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 77.525%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the deprecate_mdims branch Jan 27, 2017

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