Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typing Cleanup - Remove Blacklisted Items #25882

Closed
64 of 65 tasks
WillAyd opened this issue Mar 26, 2019 · 10 comments · Fixed by #26280
Closed
64 of 65 tasks

Typing Cleanup - Remove Blacklisted Items #25882

WillAyd opened this issue Mar 26, 2019 · 10 comments · Fixed by #26280
Labels
Typing type annotations, mypy/pyright type checking

Comments

@WillAyd
Copy link
Member

WillAyd commented Mar 26, 2019

#25844 introduced a blacklist of modules in the mypy.ini config file which currently throw errors when analyzed with mypy. We would ideally be able to whittle these down (save exclusion of the test folder(s), which I think is fine).

I'm not sure there is one easy way to split it up as the degree of difficulty may vary wildly. PRs from the community to clean up one to a few of the blacklisted modules at a time would certainly be welcome!

Here's the list of modules for reference:

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Mar 26, 2019
@WillAyd WillAyd added this to the Contributions Welcome milestone Mar 26, 2019
@GreybeardCG
Copy link

@WillAyd I've been studying a few of these files and related contributions today and was wondering if there are any catches I need to know about when fixing these mypy violations.
As an example: in array_.py:231 adding dtype = cast(ExtensionDtype, dtype)" seems the least intrusive. However, I saw that typing was added to this file partially already , so was there a specific reason to leave this violation in? Thanks a lot!

@WillAyd
Copy link
Member Author

WillAyd commented Apr 3, 2019

Have you merged master?

@GreybeardCG
Copy link

@WillAyd Yes, I'm on the latest upstream/master.

@WillAyd
Copy link
Member Author

WillAyd commented Apr 3, 2019

Sorry misread comment. We’ve had a few things on one of which was simply to compat syntax to Py3 annotation

For the module you mentioned we did that but haven’t focused on actually fixing the errors. If you still see some and want to submit a PR would certainly be welcome

@GreybeardCG
Copy link

@WillAyd All clear then! I'll keep working on it.

@gwrome
Copy link
Contributor

gwrome commented Apr 5, 2019

@WillAyd I've run into a few cases where we should use an old style type hint for a global variable to maintain pre-3.6 compatibility, but the appropriate hint is just too long for the line. I haven't been able to find any documented way to break a variable's type hint across lines or put it on another line.

Any ideas?

For example in pandas/compat/numpy/function.py:105 we want something like

ARGSORT_DEFAULTS = OrderedDict()  # type: OrderedDict[str, Union[int, str, None]]

but that line is too long. We could replace the Union with Any, but that reduces the usefulness of type checking. In this particular case, it probably doesn't matter because that ARGSORT_DEFAULTS gets fed directly into a classconstructor and is never touched again. But there may be other places in the code where accuracy is more important and the line length is an issue.

@Naddiseo
Copy link
Contributor

Naddiseo commented Apr 6, 2019

@gwrome python/mypy#4511 might be of use

tldr: type alias, or backslash at end of line

@gwrome
Copy link
Contributor

gwrome commented May 2, 2019

@WillAyd, I think these are ready for removal from the list above, because they're no longer in the blacklist. I've included the PR if I could find it:

@WillAyd
Copy link
Member Author

WillAyd commented May 2, 2019 via email

@gwrome
Copy link
Contributor

gwrome commented May 2, 2019

You’re welcome. Hoping to knock a couple more out this weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants