API: Make most arguments for read_html and read_json keyword-ony
State: closed

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


Labels:
name: IO JSON
description: read_json, to_json, json_normalize
name: IO HTML
description: read_html, to_html, Styler.apply, Styler.applymap

Comments:
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. 
> 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 ? 

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 <notifications@github.com> 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.

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?
Hello @alexitkes! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:













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

##### Comment last updated at 2020-03-31 18:09:11 UTC
Lifting https://github.com/pandas-dev/pandas/pull/27573#discussion_r308181531 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?
@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.
> 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.
@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?
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 <notifications@github.com> 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
> <https://github.com/pandas-dev/pandas/pull/27573?email_source=notifications&email_token=AAKAOIWDW7YBWLIQNA6SISTQB7ZQNA5CNFSM4IGSQB62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DFE6A#issuecomment-516313720>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIQDBCCUJD5VIZIZ5VLQB7ZQNANCNFSM4IGSQB6Q>
> .
>

> 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.
> 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.
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?
@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?
@TomAugspurger all good questions to which I don't have a definitive answer. Doing all in one release would be ideal though not sure how feasible that really is.

Knowing there's going to be exceptions to anything we do we could consider something simple like saying "any functions with more than N arguments will be keyword only" where N could be 5 or less. 
This seems to have stalled a bit.

Are we comfortable going down this API path? Any other changes needed here? (cc @gfyoung, @WillAyd when you're back).
I'm relatively comfortable with this path (it was discussed a good amount in the original issue).  However, it would be great to see some tests where we introspect on the warning message, given the amount of logic happening there.
> Having a hard time thinking through why we would want to make exceptions on a per function basis, so some of the allowances here may be overkill.

I feel somewhat similar. If it's hard to explain why some kwargs are keyword only, while others aren't, then maybe we just shouldn't do this.

(I don't really like the idea of "the first 5 are position or keyword" either 😄)
Yea totally fine. So at the moment (going back to the original issue) I *think* we either need to just break things or not do something like this as all. Trying to navigate a deprecation cycle like this probably adds a lot of cruft
Hmm I disagree with that. The deprecation seems pretty clean to me, just a
single decorator.

On Mon, Aug 26, 2019 at 3:43 PM William Ayd <notifications@github.com>
wrote:

> Yea totally fine. So at the moment (going back to the original issue) I
> *think* we either need to just break things or not do something like this
> as all. Trying to navigate a deprecation cycle like this probably adds a
> lot of cruft
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/pandas-dev/pandas/pull/27573?email_source=notifications&email_token=AAKAOIUUNCOVVEGQZLQK4LDQGQ555A5CNFSM4IGSQB62YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5FTX3Q#issuecomment-525024238>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAKAOIQREGWSISK3KCS2WPLQGQ555ANCNFSM4IGSQB6Q>
> .
>

Oh so what are your hangups on it then? I think I am misunderstanding your objections to this, if any
Mainly it's unclear to me which keyword arguments should be keyword only.

For example, if I were making the builtin `open` function use keywords, I would say that `file` and `mode` can be positional or keyword, and the rest should be keyword only. But I don't have any justification for that other than "it feels right".
I doubt there's going to be one right answer that pleases everyone, so probably just have to pick something. Biased by my own coding style I don't really see ever see much of a use for more than 1 or 2 positional arguments. Particularly in the `read_` methods only the first positional argument is ever consistent anyway
Thank you for reviews.

@gfyoung Yes, some introspection of the warning messages would be good. Unfortunately, I can't find a way to do it within `pandas.util.testing` module. Is there a proper way to examine warning messages, or it only can be done by using `warnings.catch_warnings` directly? In letter case I will still need some time to learn...

> Instead of cramming allowed_args into one variable can we have separate parameters for positional and keyword argument allowance? I think a more natural fit that way

@WillAyd Sorry, but I am afraid I don't understand what exactly you mean, possibly due to not being a native English-speaker. Can you give an example of how should a good warning message look?
@alexitkes : I would attempt to do it via `warnings.catch_warnings`.
Added some tests with warning message analysis to `test_deprecate_nonkeyword_arguments.py`.
@pandas-dev/pandas-core do we have strong thoughts on this one? Want to be cognizant of @alexitkes time if we aren't aligned

FWIW I'm not sure this is worth the effort
i would just change this and call it a day
depreciation is not worth the effort here
I also agree. I think everything after the filepath_or_buf argument should be keyword only if we do this at all

There will be some friction but its very easy to recover from and sets a consistent go-forward expectation. 1.0 is the time to do something like that too
Is the issue the complication of the deprecation, or disagreement on what should be keyword only.

I’m -1 on a breaking change. I don’t think the deprecation core complicates things much. 

No strong thoughts on the issue with what becomes keyword only. Is the fact that the builtin open method doesn’t do keyword only args helpful in deciding?

> On Oct 11, 2019, at 18:20, William Ayd <notifications@github.com> wrote:
> 
> ﻿
> I also agree. I think everything after the filepath_or_buf argument should be keyword only if we do this at all
> 
> There will be some friction but its very easy to recover from and sets a consistent go-forward expectation. 1.0 is the time to do something like that too
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.