DEPR: Deprecating Function Arguments

[gfyoung]

When we want to deprecate and remove arguments, our general methodology has been to issue the deprecation warning ONLY when a non-default argument has been passed in.

However, @jorisvandenbossche pointed out in #17820 that this warnings framework does not take into account situations where the user EXPLICITLY passes in the default argument. Thus, we risk breaking downstream code in that case.

An alternative method would be to change the default argument (to some dummy) that we can use to catch this argument when passed in with the current accepted values.

While I have not seen this case emerge (patching is relatively painless, which might be why we haven't seen this issue on our tracker), I think it's worthwhile considering as we move forward with parameter deprecations + adjusting previous ones.

[jorisvandenbossche]

I disagree with not raising for the default value is how it is typically done. I think it is rather the other way around (we normally put the deprecated kwarg to a default of None, to be able to distinguish the default and an actual user passed value). This is also how the deprecate_kwarg decorator works.

It might be that we just missed a few of those cases while reviewing in the PRs where you deprecated something, and so you have the impression that this is not the typical way deprecations are done (and likewise it is possible that I only reviewed mainly those PRs where we do it this way and thus I have the opposite impression)

Of course this issue is only relevant for a certain part of the deprecations (eg it is not relevant for deprecating/renaming kwargs that have no default value, or for changing a default value, or for deprecating/renaming a full method/class).
Let me go through the relevant ones for 0.21 release:

• read_excel sheetname -> sheet_name kwarg rename, with a default of 0: this raises the warning when you pass sheetname=0 (as it is how the deprecate_kwarg decorator works)
• take method convert kwarg: not raising for convert=True (IMO this should be fixed)
• raise_on_errors kwarg in where with default of True: raises for the default (if you look at the implementation, it actually does if raise_on_error is not None: ... [raise warning])

It is true that it is making the signature less informative, but I think we need to have this warning, as it is the purpose of the warning that people can change their code before it breaks.

[jreback]

deprecated kwargs should always be None. Then in the code you can set them to the actual value; this allows detection and will show a warning if it was passed at all.

All of the examples by @jorisvandenbossche should be fixed.

[gfyoung]

@jreback : Only the second one needs fixing. All others are according to what @jorisvandenbossche described above.

[jorisvandenbossche]

Only the second one needs fixing.

Indeed, the others were to show we generally do it like this.
But one that is not in the list is the one of read_csv deprecation of tupleize_cols (the one that actually sparked this discussion)

[jorisvandenbossche]

The original one that sparked the discussion still needs to be done I think: tupleize_cols in read_csv