REGR: merge no longer accepts a list for suffixes keyword

[jorisvandenbossche]

see #34208 (comment)

The linked PR changed the merge routines to now only accept tuples for the suffixes keyword, and no longer general sequence.

This is a breaking change (eg it breaks some GeoPandas functions), but actually also changing documented behaviour: the merge docstring indeed says "tuple", but eg merge_asof still says "tuple or list". And in our user guide, we were ourselves using a list in one example, and it explicitly says "tuple or list" (see https://pandas.pydata.org/pandas-docs/version/1.0/user_guide/merging.html#overlapping-value-columns)

In general, I am certainly fine with restricting the set of accepted types (eg to avoid the confusing issue with sets, the original report), but this is 1) breaking a documented behaviour and 2) a change that could also easily be done with a deprecation warning.
And given our versioning policy, if it is easy to do a change with a deprecation, then we should try to do that, IMO.

[jreback]

why are you opening this issue? and how is this a regression?

[jorisvandenbossche]

Maybe read my linked comment? EDIT: updated the top post to include that comment

[jreback]

i read your comment

the code was wrong and it’s now fixed
if it happened to work that’s not a reason for a deprecation warning; this is why we have release notes

[jorisvandenbossche]

It was documented to work with a list of strings.

[jreback]

nope: https://pandas.pydata.org/docs/reference/api/pandas.merge.html?highlight=merge#pandas.merge

[jorisvandenbossche]

Jeff, please read my original comment: I clearly said that the merge user guide indeed indicates that it should be a tuple (the one you linked to), but the merge_asof docstring actually mentions list as well (https://pandas.pydata.org/docs/reference/api/pandas.merge_asof.html), and the same for the user guide (https://pandas.pydata.org/pandas-docs/version/1.0/user_guide/merging.html#overlapping-value-columns)

[jreback]

and your point? deprecating for merge_asof is fine but no action for merge is warranted

just because we can deprecate doesn’t mean we should - this just leaves code around for way too long

so happy to have a patch to deprecate / fix merge_asof as an addendum (which was a doc error initially iirc)

[jorisvandenbossche]

deprecating for merge_asof is fine but no action for merge is warranted

It's documented for merge as well, in the user guide.

just because we can deprecate doesn’t mean we should - this just leaves code around for way too long

I quote our versioning policy (https://pandas.pydata.org/docs/dev/development/policies.html):

Whenever possible, a deprecation path will be provided rather than an outright breaking change.

And doing it with a deprecation would be easily possible here.

[jreback]

it doesn’t matter if it’s easy
and since when does the user guide trump the doc string itself?

as i said i am sympathetic for merge_asof but merge is a good change

if nothing broke in our test suite then there is no reason to revert / change this

geopandas needs a patch here -

[TomAugspurger]

#34208 looks like an API breaking change. We'll need to adjust for it here in pandas. Warning that we'll restrict the types in the future seems easiest (and personally I'd be fine with just checking for sets and accepting anything else, but don't have a strong opinion).

[jorisvandenbossche]

personally I'd be fine with just checking for sets and accepting anything else

+1, I think certainly lists would be perfectly fine to keep supporting

[TomAugspurger]

FWIW, this also broke Dask.

[jreback]

if u want to change to a deprecation warning ok i guess

but this should be a tuple never a list or anything else iterable

[jorisvandenbossche]

but this should be a tuple never a list or anything else iterable

Why not? What is inherently wrong about using a list for this?

[TomAugspurger]

I think we generally duck-type user arguments like this where there's no ambiguity (i.e. not indexing).