TYP: Add return types to some top-level func

[topper-123]

Adds return type hints to some top-level funcs. I will take the rest over 1-2 more PRs.

Having return types typed up will be good for the user experience (better piping etc.).

[jbrockmendel]

do we use this pattern elsewhere?

[topper-123]

I don't think so, but is the correct one because of sys.exit is used here, if I underdtand it correctly.

[topper-123]

Hmm, I see now NoReturn was only added in Python 3.6.2. We support 3.6.1 currently.

I'll remove this annotation unless peope think it's good idea to raise the python minimum version (I don't think it's a good idea myself).

[simonjayhawkins]

I think the preference is for assert instead of cast @jbrockmendel

Suggested change

	table = cast(DataFrame, filled)
	assert table is not None # needed for mypy

[topper-123]

The error is error: Incompatible types in assignment (expression has type "Optional[DataFrame]", variable has type "DataFrame"). I cant reassign table to a broader type. So that pattern won't work.

Could do

filled = table.fillna(value=fill_value, downcast="infer")
assert filled is not None
table = filled

but that's also ugly. But I agree that asserts are better, so I'll do that one above (unless you got something less ugly :-))

[simonjayhawkins]

(unless you got something less ugly :-)

we probably should be overloading all methods that have an inplace parameter, but that would require the use of Literal.

[topper-123]

Literals won’t come until 3.8 is the minimum version, so will be a while. Should we allow assignment to incompatible type in mypy, until 3.8? Seems like a lesser evil?

[topper-123]

Ok, should probably use ignore commands in the first line: https://mypy.readthedocs.io/en/latest/error_codes.html#silencing-errors-based-on-error-codes. Then ‘’table =...’’ can be used in the first line and the third line can be dropped.

I’ll update later to use this pattern.

[simonjayhawkins]

we are using the default for allow_redefinition. It does result in lots of variable renaming and some messy code, but the additional strictness is probably worth it.

[jbrockmendel]

Is there any way we could get mypy to understand that without inplace=True isna returns non-None?

[topper-123]

Not until 3.8 and Literal, AFAIK.

There has been an idea floating around that the inplace parameter should be deprecated everywhere (unless there is a performance benefit to it). That would simplify the return types.

[topper-123]

I've changed this section to use # type: ignore directive.

EDIT: didn't work, got a Flake8 F821 error (undefined name). I've reverted the commit back to the 3-line version.

[simonjayhawkins]

it's a good idea, we need to move the adoption of error codes forward. see #29197 could add # noqa for now.

[simonjayhawkins]

@topper-123 lgtm