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

Include exceptions.pyi and fields.pyi #1937

Merged
merged 5 commits into from
Sep 4, 2020

Conversation

savarin
Copy link
Contributor

@savarin savarin commented Aug 27, 2020

Addresses #1897, as an alternative to #1931.

The previous PR #1931 was created to incrementally add .pyi files from typeshed, with the following steps:

  1. Create .pyi files incrementally based on dep tree
  2. Port .pyi files in from typeshed as is
  3. Correct mypy errors flagged by nox -s lint
  4. Commit all stubs for all .pyi files created

This PR is created to illustrate another possibility, where the focus would be on porting as much of the .pyi files from typeshed first. This involves only steps 1 - 3, i.e. changes made only where errors are raised.

After all the .pyi files have been ported from typeshed, step 4 would then be completed for all the .pyi files.

@savarin
Copy link
Contributor Author

savarin commented Aug 27, 2020

@sethmlarson I'm a little concerned we might have been trying to do too much with the previous PR. I created this one in light of the objective of moving as all the .pyi files from typeshed first (and making corrections only where there are explicit mypy errors), and once that's done fill out the .pyi files. This should help us move faster (by focusing on the typeshed port first), as well as do so in a systematic way.

Steps 1 and 2 are captured in the commit f1fd1c0 (ports from typeshed as is), and step 3 in the commits b3db9ac for exceptions.pyi and 8429071 for fields.pyi. The mypy errors corrected in step 3 can be found here.

@@ -0,0 +1,55 @@
from typing import Any, Optional, Union, Tuple, TYPE_CHECKING
Copy link
Contributor Author

@savarin savarin Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From typeshed at exceptions.pyi.

@@ -0,0 +1,26 @@
# Stubs for requests.packages.urllib3.fields (Python 3.4)

from typing import Any, Callable, Dict, Optional
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From typeshed at fields.pyi.

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments

from typing import Any, Callable, Dict, Optional

def guess_content_type(filename: str, default: str) -> str: ...
def format_header_param(name: str, value: str) -> str: ...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing types for format_header_param_rfc2231 and format_header_param_html5, shouldn't Mypy catch that issue though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sethmlarson Annoyingly with stub files, mypy only catches errors on a subsequent import. For example https://github.com/savarin/urllib3/compare/port-types-03...savarin:port-types-03-example?expand=1 would raise the following error.

src/urllib3/result.py:4: error: Argument 2 to "format_header_param" has incompatible type "int"; expected "str".

Types added for those two.

name: str,
data: Any,
filename: Optional[str],
headers: Optional[Dict[str, str]],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers: Optional[Mapping[str, str]]

@savarin
Copy link
Contributor Author

savarin commented Aug 30, 2020

@sethmlarson Changes incorporated.

I also have an idea on tracking progress. We can simply count the lines returned by mypy (as per 20d00cc) to get a sense how many errors we're reducing at each change.

# Remove all existing `.pyi` files
nox > all errors count: 1116

# Current master
nox > all errors count: 1105

# This PR
nox > all errors count: 854

@codecov
Copy link

codecov bot commented Aug 30, 2020

Codecov Report

Merging #1937 into master will increase coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1937      +/-   ##
==========================================
+ Coverage   99.72%   99.81%   +0.09%     
==========================================
  Files          24       24              
  Lines        2179     2179              
==========================================
+ Hits         2173     2175       +2     
+ Misses          6        4       -2     
Flag Coverage Δ
#unittests 99.35% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/urllib3/util/ssl_.py 100.00% <0.00%> (+2.24%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf6ab7a...20d00cc. Read the comment docs.

@sethmlarson
Copy link
Member

Hey @savarin wanted to thank you for all this work. Want to keep you up to date with what we're planning and how that relates to type hints.

Right now I'm planning on shipping type hint support in v2 which likely won't be released until mid/late 2021. For now you can continue with type stubs and when the master branch becomes the "v2" branch we'll migrate your type stubs into inline type hints. This means that type stubs won't get released in v1.26 because our API will become a lot simpler when we drop Python 2.7 support. So for now:

  • You can continue as you're doing with .pyi files, we'll handle the rolling into inline type hints via a tool probably
  • Focus on Python 3.6+ types, don't worry about urllib3.packages.six or code that's written explicitly for Python 2 for example.
  • If you are discouraged by this I understand, we really value what you're doing and all hope you'll still be motivated to keep contributing. Let me know if you want to discuss more via email (sethmichaellarson@gmail.com)

@savarin
Copy link
Contributor Author

savarin commented Sep 2, 2020

Hey @savarin wanted to thank you for all this work. Want to keep you up to date with what we're planning and how that relates to type hints.

Right now I'm planning on shipping type hint support in v2 which likely won't be released until mid/late 2021. For now you can continue with type stubs and when the master branch becomes the "v2" branch we'll migrate your type stubs into inline type hints. This means that type stubs won't get released in v1.26 because our API will become a lot simpler when we drop Python 2.7 support. So for now:

  • You can continue as you're doing with .pyi files, we'll handle the rolling into inline type hints via a tool probably
  • Focus on Python 3.6+ types, don't worry about urllib3.packages.six or code that's written explicitly for Python 2 for example.
  • If you are discouraged by this I understand, we really value what you're doing and all hope you'll still be motivated to keep contributing. Let me know if you want to discuss more via email (sethmichaellarson@gmail.com)

@sethmlarson Thanks for sharing and the kind words. Re: plans, it's not a problem. Focus on 3.6+ types makes a lot of sense. Let me know if you have any further thoughts/comments on this PR.

Copy link
Member

@sethmlarson sethmlarson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants