API: Make most arguments for read_html and read_json keyword-ony #27573

alexitkes · 2019-07-24T17:56:02Z

As mentioned in #27544 it is better to use keyword-only arguments for functions with too many arguments. A deprecation warning will be displayed if read_html get more than 2 positional arguments (io and match) or read_json get more than 1 positional argument (path_or_buf).

tests added / passed
Three groups of tests are actually needed
- Tests dof the deprecate_nonkeyword_arguments decoratorr
- Check whether the read_json function emits a FutureWarning whenever necessary and does not emit it whenever not.
- Check whether the read_html function emits a FutureWarning whenever necessary and does not emit it whenever not.
passes black pandas
passes git diff upstream/master -u -- "*.py" | flake8 --diff
whatsnew entry

TomAugspurger · 2019-07-25T12:08:31Z

We need to deprecate the old behavior first. As written, this is an api breaking change.

And we shouldn’t do this until 1.0 or later.

jreback · 2019-07-25T12:44:41Z

We need to deprecate the old behavior first. As written, this is an api breaking change.

And we shouldn’t do this until 1.0 or later.

and how exactly would you deprecate ?

TomAugspurger · 2019-07-25T12:57:55Z

Should be able to use a decorator to inspect the calm, right? And if any positional or keyword args are passed in args then we warn?

…

On Jul 25, 2019, at 07:44, Jeff Reback ***@***.***> wrote: We need to deprecate the old behavior first. As written, this is an api breaking change. And we shouldn’t do this until 1.0 or later. and how exactly would you deprecate ? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

alexitkes · 2019-07-25T13:32:31Z

Thank you. I also wanted to propose somewhat like this.

def Deprecated(*, n_args: int):
    def _deprecated(f):
        def _f(*args, **kwargs):
            if len(args) > n_args:
                warnings.warn("%s will only accept %i positional arguments in version X.Y.Z" % (f.__name__,n_args))
            return f(*args, **kwargs)
        return _f
    return _deprecated

Or do such a decorator already exist?

pep8speaks · 2019-07-26T19:49:27Z

Hello @alexitkes! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-03-31 18:09:11 UTC

TomAugspurger

Can you add tests to read_html / read_json to ensure that warnings are raised there?

doc/source/whatsnew/v0.25.1.rst

pandas/io/html.py

TomAugspurger · 2019-07-29T17:26:55Z

Lifting #27573 (comment) to the top level

I'd like not to repeat the arguments here. That seems fragile. Is it possible to have the decorator inspect the signature and warn on any positional_or_keyword arg passed positionally?

What do you think about this @alexitkes?

alexitkes · 2019-07-29T18:49:13Z

@TomAugspurger Thank you for review. Yes, writing argument list twice will be quite annoying, but I will need some time to find a way to avoid it, and I would like to try. However, I think it would be better to preserve the allowed_args argument for the decorator and default it to list of required arguments if None given.

TomAugspurger · 2019-07-29T22:22:13Z

However, I think it would be better to preserve the allowed_args argument for the decorator and default it to list of required arguments if None given.

Why do you think we'll need that? That sounds more complicated than just detecting kwargs passed positionally. If we aren't going to need it then I'd rather not implement it.

alexitkes · 2019-07-30T08:10:36Z

@TomAugspurger I have just written such a decorator and it does not look too complicated. However it is my first experience with inspect module so possibly more tests are needed though some tests are written and they all run successfully.

It will not be difficult to remove allowed_args argument and always set it to list of required arguments, if you think eventually all arguments having the default values should be keyword-only arguments. Should I do it?

TomAugspurger · 2019-07-30T11:37:03Z

I'm not sure without looking more closely. My thought is that all keyword arguments being keyword-only is easier to explain to users.

…

On Tue, Jul 30, 2019 at 3:10 AM Alex Itkes ***@***.***> wrote: @TomAugspurger <https://github.com/TomAugspurger> I have just written such a decorator and it does not look too complicated. However it is my first experience with inspect module so possibly more tests are needed though some tests are written and they all run successfully. It will not be difficult to remove allowed_args argument and always set it to list of required arguments, if you think eventually all arguments having the default values should be keyword-only arguments. Should I do it? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#27573?email_source=notifications&email_token=AAKAOIWDW7YBWLIQNA6SISTQB7ZQNA5CNFSM4IGSQB62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DFE6A#issuecomment-516313720>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAKAOIQDBCCUJD5VIZIZ5VLQB7ZQNANCNFSM4IGSQB6Q> .

alexitkes · 2019-07-30T12:51:35Z

I'm not sure without looking more closely. My thought is that all keyword
arguments being keyword-only is easier to explain to users.

I have almost forgotten one thing - the path_or_buf argument for read_json has the default value None. So if we decorate the function in that way all arguments for read_json will be keyword-only. But I think most users are used to writing read_json(filename) and will not like to write read_json(path_or_buf=filename). For this reason I still think there should be a way to pass the list of arguments allowed to be positional to the decorator. However, the default decorator behavior (optional arguments are keyword only) should be used wherever there is no really significant reason to change it. The read_html function is decorated in this way.

TomAugspurger · 2019-07-30T13:38:19Z

I have almost forgotten one thing - the path_or_buf argument for read_json has the default value None. So if we decorate the function in that way all arguments for read_json will be keyword-only.

Oh sorry. In that case we'll definitely need to have the ability to have some keyword-or-positional arguments.

alexitkes · 2019-07-30T19:40:36Z

Well, it seems to work fine though I am a bit worried because it's my first experience with inspect module. I also don't know the proper number of version in which it will become impossible to use positionl arguments (currently written 1.1). What should it be?

pandas/io/html.py

TomAugspurger · 2019-08-05T16:27:03Z

pandas/tests/io/json/test_deprecated_kwargs.py

+    """
+    df = pd.DataFrame({"A": [2, 4, 6], "B": [3, 6, 9]}, index=[0, 1, 2])
+    with tm.assert_produces_warning(FutureWarning):
+        assert_frame_equal(df, read_json(df.to_json(orient="split"), "split"))


Can you minimize the amount of code in the context manager? I think just read_json(buf, "split").

Also, do we need to test all three of these? What differs between them?

pandas/tests/io/json/test_deprecated_kwargs.py

pandas/util/_decorators.py

TomAugspurger · 2019-08-05T16:29:26Z

pandas/util/_decorators.py

+        def wrapper(*args, **kwargs):
+            if isinstance(allow_args, int) and len(args) > allow_args:
+                msg = (
+                    "After version %s all arguments of %s "


version -> pandas version.

Use {}-style string templates.

Ok, done it. I also added a separate function to form a better looking warning message depending on the number of arguments, but so far I only tested it manually. Is there a proper way to test how exactly does the produced warning look?

pandas/tests/util/test_deprecate_nonkeyword_arguments.py

TomAugspurger · 2019-08-06T12:50:58Z

@WillAyd @jreback @jorisvandenbossche do you have thoughts on this (the overall goal, the implementation seems fine).

Do we expect to expand this to more readers? Should these be done in the same release?
What about other places in the library? Same release?
What's a good general policy on whether an argument should be keyword only?

pandas/tests/io/test_html.py

WillAyd · 2019-08-06T14:56:21Z

pandas/tests/io/test_html.py

    def test_index(self):
-        df1 = self.read_html(self.spam_data, ".*Water.*", index_col=0)
-        df2 = self.read_html(self.spam_data, "Unit", index_col=0)
+        with tm.assert_produces_warning(FutureWarning):


Similar comment for non-duplicated tests; would rather not use the deprecated behavior

Well, such amount of duplicated tests looks quite ugly, so I removed all tests with deprecated behavior except for one. Would it be enough?

pandas/tests/util/test_deprecate_nonkeyword_arguments.py

pandas/util/_decorators.py

pandas/io/html.py

Co-Authored-By: Joris Van den Bossche <jorisvandenbossche@gmail.com>

alexitkes · 2020-03-31T18:57:07Z

@WillAyd OK, done it.

WillAyd

I would be OK with moving forward here - @jreback or @TomAugspurger ?

TomAugspurger · 2020-04-02T01:35:42Z

Do we have a general policy for which arguments are keyword only?

jreback · 2020-04-03T22:03:44Z

lgtm. on anything with lots of args (pretty much read_*), happy to only have the first arg positional (path_or_buf).

jreback · 2020-04-06T23:25:44Z

looks good, @TomAugspurger ok here?

TomAugspurger · 2020-04-06T23:46:21Z

Yeah

…

On Apr 6, 2020, at 18:25, Jeff Reback ***@***.***> wrote: looks good, @TomAugspurger ok here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jreback · 2020-04-07T00:55:42Z

thanks!

jorisvandenbossche · 2020-04-07T08:08:42Z

Thanks @alexitkes !

simonjayhawkins added IO HTML read_html, to_html, Styler.apply, Styler.applymap IO JSON read_json, to_json, json_normalize labels Jul 24, 2019

alexitkes force-pushed the alexitkes/keyword-args branch from 22fd9dc to 5318a76 Compare July 24, 2019 19:47

alexitkes changed the title ~~WIP: Make most arguments for read_html and read_json keyword-ony~~ Make most arguments for read_html and read_json keyword-ony Jul 24, 2019

alexitkes changed the title ~~Make most arguments for read_html and read_json keyword-ony~~ [WIPMake most arguments for read_html and read_json keyword-ony Jul 26, 2019

alexitkes changed the title ~~[WIPMake most arguments for read_html and read_json keyword-ony~~ [WIP] Make most arguments for read_html and read_json keyword-ony Jul 26, 2019

alexitkes force-pushed the alexitkes/keyword-args branch from b00ec4a to 99e0e3c Compare July 28, 2019 15:46

alexitkes changed the title ~~[WIP] Make most arguments for read_html and read_json keyword-ony~~ Make most arguments for read_html and read_json keyword-ony Jul 28, 2019

TomAugspurger reviewed Jul 29, 2019

View reviewed changes

doc/source/whatsnew/v0.25.1.rst Outdated Show resolved Hide resolved

pandas/io/html.py Outdated Show resolved Hide resolved

alexitkes force-pushed the alexitkes/keyword-args branch from d6f29f4 to cd5d658 Compare July 29, 2019 13:29

TomAugspurger mentioned this pull request Jul 29, 2019

use keyword only args where appropriate #27544

Closed

TomAugspurger reviewed Aug 5, 2019

View reviewed changes

alexitkes force-pushed the alexitkes/keyword-args branch from 2982165 to f737870 Compare August 6, 2019 08:42

alexitkes force-pushed the alexitkes/keyword-args branch 2 times, most recently from cd84833 to c8284ef Compare August 6, 2019 12:57

WillAyd requested changes Aug 6, 2019

View reviewed changes

alexitkes force-pushed the alexitkes/keyword-args branch from 46e9ceb to 70359f6 Compare March 31, 2020 15:06

alexitkes and others added 13 commits March 31, 2020 21:08

Deprecate use of most positional arguments for read_html and read_json

7d16272

Import pandas._testing instead of pandas.util.testing

667e29e

Import pandas._testing instead of pandas.util.testing

93f9c20

Update pandas/util/_decorators.py

1baeceb

Co-Authored-By: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Change displayed warning message

183ab83

Update pandas/io/html.py

37f54ac

Co-Authored-By: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Update pandas/io/json/_json.py

0650ae9

Co-Authored-By: Joris Van den Bossche <jorisvandenbossche@gmail.com>

Restore permissions to v1.0.0.rst

6c83e33

Fix expected warning message in tests for deprecate_nonkeyword_arguments

ce1f251

Reformat too long line

07817fc

Remove a test too similar to another one.

40a0674

Update Whatsnew

251ba47

Fix linting

ed63145

alexitkes force-pushed the alexitkes/keyword-args branch from 01ec83a to ed63145 Compare March 31, 2020 18:09

WillAyd approved these changes Apr 2, 2020

View reviewed changes

jreback added this to the 1.1 milestone Apr 6, 2020

jreback approved these changes Apr 6, 2020

View reviewed changes

jorisvandenbossche mentioned this pull request Apr 6, 2020

DEPR: log of deprecations in 1.x (to be removed in 2.0) #30228

Closed

jreback merged commit 467e1c2 into pandas-dev:master Apr 7, 2020

shoyer mentioned this pull request May 19, 2020

Most arguments to open_dataset should be keyword only pydata/xarray#4080

Closed

topper-123 mentioned this pull request May 27, 2020

DEPR: deprecate non keyword arguments in read_excel #34418

Merged

gcaria mentioned this pull request Jul 16, 2021

Keyword only args for arguments like "drop" pydata/xarray#5531

Closed

Uh oh!

API: Make most arguments for read_html and read_json keyword-ony #27573

API: Make most arguments for read_html and read_json keyword-ony #27573

Uh oh!

Conversation

alexitkes commented Jul 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TomAugspurger commented Jul 25, 2019

Uh oh!

jreback commented Jul 25, 2019

Uh oh!

TomAugspurger commented Jul 25, 2019 via email

Uh oh!

alexitkes commented Jul 25, 2019

Uh oh!

pep8speaks commented Jul 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-03-31 18:09:11 UTC

Uh oh!

TomAugspurger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TomAugspurger commented Jul 29, 2019

Uh oh!

alexitkes commented Jul 29, 2019

Uh oh!

TomAugspurger commented Jul 29, 2019

Uh oh!

alexitkes commented Jul 30, 2019

Uh oh!

TomAugspurger commented Jul 30, 2019 via email

Uh oh!

alexitkes commented Jul 30, 2019

Uh oh!

TomAugspurger commented Jul 30, 2019

Uh oh!

alexitkes commented Jul 30, 2019

Uh oh!

Uh oh!

TomAugspurger Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TomAugspurger Aug 5, 2019

Choose a reason for hiding this comment

Uh oh!

alexitkes Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomAugspurger commented Aug 6, 2019

Uh oh!

Uh oh!

WillAyd Aug 6, 2019

Choose a reason for hiding this comment

Uh oh!

alexitkes Aug 7, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexitkes commented Mar 31, 2020

Uh oh!

WillAyd left a comment

Choose a reason for hiding this comment

Uh oh!

TomAugspurger commented Apr 2, 2020

Uh oh!

jreback commented Apr 3, 2020

Uh oh!

jreback commented Apr 6, 2020

Uh oh!

TomAugspurger commented Apr 6, 2020 via email

Uh oh!

alexitkes commented Jul 24, 2019 •

edited

Loading

pep8speaks commented Jul 26, 2019 •

edited

Loading