CLN: Assorted typings #28604
CLN: Assorted typings #28604
Conversation
| index: bool = True, | ||
| encoding: str = "utf8", | ||
| hash_key=None, | ||
| categorize: bool = True, |
There was a problem hiding this comment.
adding type hints here is causing pandas/core/util/hashing.py:132: error: Incompatible types in assignment (expression has type "chain[Any]", variable has type "Generator[Any, None, None]") further down as the body is now checked.
There was a problem hiding this comment.
it looks like the conda environment now has mypy 0.720. result!
fix is here 304351e is you want to include in the PR. (can't use py3.6 variable annotations though.)
There was a problem hiding this comment.
@jbrockmendel i was a bit hasty in my approval. a couple of new mypy errors arising from adding additional type hints
pandas/core/algorithms.py:1938: error: Incompatible types in assignment (expression has type "Tuple[slice, ...]", variable has type "List[slice]")
pandas/core/algorithms.py:1942: error: Incompatible types in assignment (expression has type "Tuple[slice, ...]", variable has type "List[slice]")
|
|
||
| # keep `hashes` specifically a generator to keep mypy happy | ||
| _hashes = itertools.chain(hashes, index_hash_generator) | ||
| hashes = (x for x in _hashes) |
There was a problem hiding this comment.
you can also do a cast here (and in the change above), not sure what is more idiomatic & what we have settled on
There was a problem hiding this comment.
you can also do a
casthere
Our type hints policy is "..., the use of cast is strongly discouraged. Where applicable a refactor of the code to appease static analysis is preferable". https://dev.pandas.io/docs/development/contributing.html#style-guidelines
personally I'm not a fan of changing the runtime behavior of code just to appease mypy if not absolutely necessary unless it makes for cleaner code. (This applies to both our policy and the changes in this PR)
The problem in this function is because Mypy considers the initial assignment as the definition of a variable. https://mypy.readthedocs.io/en/stable/type_inference_and_annotations.html#type-inference which occurs on L114.
mypy infers the type as Generator[Any, None, None].
it is possible to override the inferred type of a variable by using a variable type annotation https://mypy.readthedocs.io/en/stable/type_inference_and_annotations.html#explicit-types-for-variables
This is the approach I took to silence this mypy error in 304351e. I prefer this as it has no effect on the runtime behavior.
with regard to the use of a leading underscore to avoid Incompatible types in assignment.. errors, my preference would be to use a trailing underscore. In PEP8, a trailing underscore is used by convention to avoid conflicts with Python keywords. So I see more similarity here to avoid conflicts than the leading underscore idiom. Not sure what's best here, but just my POV if we want to choose a pattern to be consistent moving forward.
There was a problem hiding this comment.
It looks like the main actionable suggestion here is to change _hashes to hashes_; is that right @simonjayhawkins ? I'm fine with that.
|
Thanks @jbrockmendel |
Broken off from abandoned local branches.
Also fixes one of the problems currently afflicting the CI in tests.groupby.test_categorical