26302 add typing to assert star equal funcs
State: closed

- [X] closes #26302 
- [X] passes `black pandas`
- [X] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`



Labels:
name: Typing
description: type annotations, mypy/pyright type checking

Comments:
OK this is ready for another review. I believe I've addressed all your comments. 
Looks like CI is failing - can you take a look?
CI is passing now. 
lgtm. @simonjayhawkins @WillAyd 

any way we can get around having to cast in lots of places?
> any way we can get around having to cast in lots of places?

we need a cast nearly every time a is_* method is called.

mypy can't perform type narrowing in these cases. would need to use an isinstance check instead to avoid casts (for nominal subtypes).

maybe Protocol could be used for structural subtypes see https://mypy.readthedocs.io/en/latest/protocols.html#simple-user-defined-protocols or wait til py3.8 is our minimum version?
what is the status here? reading comments seems like we should type inputs as Any and then cast?
> what is the status here? reading comments seems like we should type inputs as Any and then cast?

if we type input as Any, we wouldn't need to cast (nor would anyone else using stubgen to typecheck their code, since the assert_* functions is part of the public API)
> > what is the status here? reading comments seems like we should type inputs as Any and then cast?
> 
> if we type input as Any, we wouldn't need to cast (nor would anyone else using stubgen to typecheck their code, since the assert_* functions is part of the public API)

ok then is PR what you are expecting? so some errors now would be caught at type checking time rather than at test time, e.g. you can't pass a scalar to a Series typed assert function for example (assume that you can infer the type to a scalar for example)
> ok then is PR what you are expecting?

not at present


> so some errors now would be caught at type checking time rather than at test time, e.g. you can't pass a scalar to a Series typed assert function for example (assume that you can infer the type to a scalar for example)

if the intent is to pass a Series to assert_series_equal somewher in the codebase, then no this won't be picked up if we change the types to Any.

with regard to potential coding errors in the assert functions themselves;

we tend to use a `_check_isinstance` helper in the assert functions, so could probably add a cast in those functions after the `_check_isinstance` call to perform type narrowing and allow stricter static type checking of the bodies of the assert functions. This could be a follow-on though.

Just to be clear, @simonjayhawkins, you want all the assert_*_equal functions to have type left: Any, right: Any? 
> Just to be clear, @simonjayhawkins, you want all the assert_*_equal functions to have type left: Any, right: Any?

yes. (or `object` or left untyped)
@samuelsinayoko can you fix merge conflict and incorporate feedback from @simonjayhawkins ?
can you merge master and will look again
Will do in the next couple days.


> On 27 Dec 2019, at 19:52, Jeff Reback <notifications@github.com> wrote:
> 
> ﻿
> can you merge master and will look again
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

@samuelsinayoko do you have time to merge master, and check last comments in the discussion?
Sorry I haven’t had a chance to review this yet. Will take a look tonight. 

> On 6 Jan 2020, at 12:16, Marc Garcia <notifications@github.com> wrote:
> 
> ﻿
> @samuelsinayoko do you have time to merge master, and check last comments in the discussion?
> 
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.

Hello @samuelsinayoko! 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:

* In the file [`pandas/util/testing.py`](https://github.com/pandas-dev/pandas/blob/4a19f0aadd38124dee6acdf3d1148149658652c5/pandas/util/testing.py):

> [Line 266:89](https://github.com/pandas-dev/pandas/blob/4a19f0aadd38124dee6acdf3d1148149658652c5/pandas/util/testing.py#L266): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (90 > 88 characters)
> [Line 276:89](https://github.com/pandas-dev/pandas/blob/4a19f0aadd38124dee6acdf3d1148149658652c5/pandas/util/testing.py#L276): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (90 > 88 characters)
> [Line 862:89](https://github.com/pandas-dev/pandas/blob/4a19f0aadd38124dee6acdf3d1148149658652c5/pandas/util/testing.py#L862): [E501](https://duckduckgo.com/?q=pep8%20E501) line too long (90 > 88 characters)


> > Just to be clear, @simonjayhawkins, you want all the assert_*_equal functions to have type left: Any, right: Any?
> 
> yes. (or `object` or left untyped)

I think we should just close this PR then. As you say if you want the types to be Any, or object, or just left untyped then we don't need this PR. 

Personally I think we should be more strict rather than less, especially in tests, so obvious bugs are caught at compile time by mypy rather than propagate further down the code and cause an exception at runtime. If a function is of the form `assert_series_equal(a, b)`, I wouldn't expect it to pass for anything other than two series. These two objects are never going to be equal if they're not both series. 


As a side note: typing the left and right parameters of the assert* methods will cause many false positives as the return types of many methods are unions (or optional) and cannot be typed more exact without utilizing Literal, which is in py3.8. we are still supporting 3.6.

```python
import numpy as np

import pandas as pd
import pandas.testing as tm

# optional return type

df = pd.DataFrame([[np.nan, 2, np.nan, 0]])
result = df.fillna("foo")
expected = pd.DataFrame([["foo", 2, "foo", 0]])
tm.assert_frame_equal(result, expected)

# union return type

ser = pd.Series([1, 2, 3])
result2 = pd.concat([ser, ser], axis=1)
expected2 = pd.DataFrame([[1, 1], [2, 2], [3, 3]])
tm.assert_frame_equal(result2, expected2)
```

test.py:11: error: Argument 1 to "assert_frame_equal" has incompatible type "Optional[DataFrame]"; expected "DataFrame"
test.py:18: error: Argument 1 to "assert_frame_equal" has incompatible type "Union[DataFrame, Series]"; expected "DataFrame"

Can we not just make the left argument something like `Optional[NDFrame]` (replace where applicable) and the right argument `object`? 

This one has been open for a while. This isn't user facing code so I don't think need to split hairs
@samuelsinayoko is this still active? What do you think about previous suggestion?
Closing as stale but ping if you'd like to pick back up