Restrict Pandas merge suffixes param type to list/tuple to avoid interchange in right and left suffix order
State: closed

- [x] closes #33740
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`


Labels:
name: Reshaping
description: Concat, Merge/Join, Stack/Unstack, Explode
name: API Design
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
Thanks @PuneethaPai for the PR. can you explain the changes? why not just validate in `merge` in pandas\core\reshape\merge.py? Can you also explain the benefit of adding list as an acceptable type? A two tuple can be statically type checked, whereas a list requires runtime length validation.
HI @simonjayhawkins ,
- Added list also to acceptable type, as many tests written in [test_merge.py](https://github.com/pandas-dev/pandas/blob/master/pandas/tests/reshape/merge/test_merge.py), we have used both list and tuple for suffixes.
- Had to introduce type checking as people should not pass `set` which is causing change in order, as `set` returns sorted elements when you unpack.
- Added length validation error tests as no one had done that in tests. But this is not really part of bug #33740.
- I can move the validation in merge method it-self before creating `_MergeOperation` object. Do let me know.

> A two tuple can be statically type checked

did you refer to static checking tools for python like pylint, pyflake, etc?
> whereas a list requires runtime length validation.

I think both list/tuple needs length check. I haven't done length checking explicitly. Relying on python to through error. Example:
```python
In [16]: a, b = (1,2,3)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-16-22772806e702> in <module>
----> 1 a, b = (1,2,3)

ValueError: too many values to unpack (expected 2)

In [17]: a, b = [1, 2, 3]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-17-c702cf699c19> in <module>
----> 1 a, b = [1, 2, 3]

ValueError: too many values to unpack (expected 2)

In [18]: a, b = [1]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-18-605f28859c6e> in <module>
----> 1 a, b = [1]

ValueError: not enough values to unpack (expected 2, got 1)
```

I have added tests for these error messages that's all.
> * Added list also to acceptable type, as many tests written in [test_merge.py](https://github.com/pandas-dev/pandas/blob/master/pandas/tests/reshape/merge/test_merge.py), we have used both list and tuple for suffixes.

the docs clearly state tuple, so I would be ok with changing the tests. need to wait and see what others think.

> * Had to introduce type checking as people should not pass `set` which is causing change in order, as `set` returns sorted elements when you unpack.

again docs clearly state tuple, so could be regarded as a user error, but ok with validating parameters and raising exception to avoid unexpected results.

> * Added length validation error tests as no one had done that in tests. But this is not really part of bug #33740.

sgtm

> * I can move the validation in merge method it-self before creating `_MergeOperation` object. Do let me know.

it seems logical to me minimise the changes to validate prior to the code removed, L670 in pandas/core/reshape/merge.py 

The other changes do reduce the number of parameters passed around, but is that cleanup required here. My preference is to keep PRs atomic where possible. again wait and see what others think.



> > A two tuple can be statically type checked
> 
> did you refer to static checking tools for python like pylint, pyflake, etc?

thinking mypy ( and end users using mypy)

> I think both list/tuple needs length check. I haven't done length checking explicitly. Relying on python to through error. Example:

sgtm.
Hello @PuneethaPai! 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-06-06 04:03:24 UTC
@simonjayhawkins  or @jreback ,
> Line 2059:74: E231 missing whitespace after ',' 

flake8 as you can see above asks me to add white space after `,`.

But black doesn't like this.
```bash
black --diff pandas/tests/reshape/merge/test_merge.py 
--- pandas/tests/reshape/merge/test_merge.py    2020-05-20 03:05:31 +0000
+++ pandas/tests/reshape/merge/test_merge.py    2020-05-20 03:14:29.050606 +0000
@@ -2054,11 +2054,11 @@
     tm.assert_frame_equal(result, expected)
 
 
 @pytest.mark.parametrize(
     "col1, col2, suffixes",
-    [("a", "a", (None, None)), ("a", "a", ("", None)), (0, 0, (None, "")), ],
+    [("a", "a", (None, None)), ("a", "a", ("", None)), (0, 0, (None, "")),],
 )
 def test_merge_suffix_error(col1, col2, suffixes):
     # issue: 24782
     a = pd.DataFrame({col1: [1, 2, 3]})
     b = pd.DataFrame({col2: [3, 4, 5]})
reformatted pandas/tests/reshape/merge/test_merge.py
All done! ✨ 🍰 ✨
1 file reformatted.
```

As you can see it will delete white space. Can anyone of you suggest fix for this?
This PR is stuck on this simple issue. 😸 

Thanks @simonjayhawkins . I wasn't able to debug the issue as I couldn't run that ci stage in my local. Thanks for identifying bug.
@PuneethaPai I _think_ we are going to need a note in the Backwards incompatible API changes
section of doc\source\whatsnew\v1.1.0.rst, something along the line that the parameter is now checked and an exception raised, whereas before a list or set were accepted and that the set could produce unexpected results and add the issue number.
Hi, @simonjayhawkins and @jreback
I have refreshed the pull request to resolve merge conflict. Hope it's all good now.
Sorry for multiple to-and-fro conversation. Appreciate your support.

Thanks
thanks @PuneethaPai nice!
This is breaking GeoPandas (where we are using list and not tuple). 

> the docs clearly state tuple

That's not *fully* correct. The `merge` docstring indeed say tuple, but eg `merge_asof` still says "tuple or list". And in our user guide, we were ourselves using a list.

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 also a change that could 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.