Skip to content
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

DEPR: Deprecate cols in several functions #6693

Closed
wants to merge 1 commit into from

Conversation

jsexauer
Copy link
Contributor

closes #6680 -- Deprecate cols in drop_duplicates() and duplicated() (rename to subset)
closes #6645 -- Deprecate cols in to_csv() and to_excel() (rename to columns)
releated to #6686
Related #5505 -- Deprecate cols and rows in pivot_table() and crosstab() (rename to columns and index)

Related #6581 -- Reminder to come back in 0.16, remove deprecation warnings, and use new arguments

@jreback
Copy link
Contributor

jreback commented Mar 23, 2014

maybe should refactor the code that does this cols deprecation check to a function (and also need to change the ones u did already, eg pivot and such)

maybe stick it in pandas.util.misc or something (their is a decorator deprecate already in pandas.util.decorators but not sure if that would work or maybe it would)

@jsexauer jsexauer changed the title Change cols to subset in drop_duplicates and duplicated DEPR: Deprecate cols in several functions Mar 23, 2014
@jreback jreback added this to the 0.14.0 milestone Mar 23, 2014
@jsexauer
Copy link
Contributor Author

Made a new decorator that deprecates a keyword argument of a function and implemented on the 6 functions in question.

@@ -178,6 +178,16 @@ These are out-of-bounds selections
``FutureWarning`` is raised to alert that the old ``rows`` and ``cols`` arguments
will not be supported in a future release (:issue:`5505`)

- The :meth:`DataFrame.drop_duplicates` and :meth:`DataFrame.duplicated` methods
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you move these to the deprecations section

@jsexauer
Copy link
Contributor Author

Thanks for catching the bone-headed overlook of not using the decorator on to_csv and to_excel

@jreback
Copy link
Contributor

jreback commented Mar 24, 2014

looks good

@jorisvandenbossche ?

@jorisvandenbossche
Copy link
Member

There should be some wrapping of the function in order to keep its name and docstring.

raise TypeError(msg)
else:
kwargs[new_arg_name] = old_arg_value
return func(*args, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use functools.wraps to propogate name/docstrings

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying this, but I first couldn't get it to work (with the double wrapper for providing the arguments, but now found where the wrap should come. It's like this:

from functools import wrap
...
def deprecate_kwarg(old_arg_name, new_arg_name):
    def _deprecate_kwarg(func):
        @wraps(func)                       # <-----
        def wrapper(*args, **kwargs):
            ...
            return func(*args, **kwargs)
        return wrapper
    return _deprecate_kwarg

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just assign directly, e.g func.__name__ = ....; func.__doc__ = ....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jreback Is there a downside to using functools.wraps?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no....its more future proof...so ideally use it over manual assigning....but usually name/doc good enough

@jankatins
Copy link
Contributor

wouldn't it make sense to add a version to the decorator, so that this information can be printed as well and maybe make this fail in case some env variable is present (TRAVIS) and pandas version is higher (automatic failures on version update :-) )

@jreback
Copy link
Contributor

jreback commented Mar 24, 2014

@JanSchulz why would you need a version in this - its already versioned by definition as its in master.

Even though we have created an issue to take this out for good (say in 0.16), that might get changed until its actually removed. Their are some very old deprecations thatreally do need to get taken out.

@jankatins
Copy link
Contributor

I mean something like this: deprecate_kwarg(old_arg_name="old", new_arg_name="new", version="0.5") will output "old is deprecated, use new instead (old will be removed in version 0.5)" and tests using old will fail on travis if pd.version is higher than 0.5.

@jreback
Copy link
Contributor

jreback commented Mar 24, 2014

can't do it because what happens when that 0.5 changes to 0.6 (eg we didn't remove it when we said but later)

it's not always clear how long to deprecate and when to remove

in theory you are right but practically it's a problem

…Use decorator. Update docs and unit tests. [fix pandas-dev#6645, fix#6680]
@jsexauer
Copy link
Contributor Author

Added wraps decorator

@jreback
Copy link
Contributor

jreback commented Mar 25, 2014

merged via 599156f

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecate Functionality to remove in pandas
Projects
None yet
4 participants