Add missing attribute hints for click.types #4813

max-frank · 2020-12-13T17:57:01Z

This PR adds attribute type hints for known attributes of click.types.
Current release click 7.1.2 was used as reference.

PR in response to mypy errors experienced in mkdocs/mkdocs-click#25

max-frank · 2020-12-13T18:19:02Z

third_party/2and3/click/types.pyi

+    readable: bool
+    resolve_path: bool
+    allow_dash: bool
+    type: Optional[Type[_PathType]]


Not sure on the typing for this causes the tests to fail.
I already tried fixing it according to the hints produced by the failing test, but had not luck.
Please advise

The typevar doesn't make sense in this context, since there's nothing to bind it. Perhaps you want Optional[Union[Type[str], Type[bytes]]].

I see it seems I slightly misunderstood TypeVars and binding until now. I've added the type as per your suggestion since I also believe this to be the correct hint for this. For this I added it as an alias _PathTypeBound just below the _PathType TypeVar. As to prevent possible future type changes only being done to the _PathType init argument and not the attribute, especially since they argument and attribute do not share the exact same name.

Also rebased to the latest master containing the proposed fix for #4815 .

The typevar is actually already broken in the current version: It's used in two different functions without the class being generic. I see two possible solutions:

Make the class generic over the path type and use the typevar in all three positions.

Use the union in all three positions.

I have a slight preference for the first option, although with the usual caveat that this requires every user of Path to annotate it. (At least as long as we don't have support for generic defaults.) Cc @JelleZijlstra.

Making the class generic seems to make sense; in that case we should add an overload to the __init__ function to specify the value of the typevar when the path argument isn't passed.

@max-frank Do you want to change it? Otherwise I would merge the PR as is and send a separate PR myself.

@srittau Ah yes sorry I was doing a deep dive into the click code base and got a bit side tracked.
Yes please, if you wan't you can do a separate PR I was just in the process of figuring out what the default would actually be when path_type=None.

From what I can see it should boil down to the following two cases depending on the system

unicode: for PY2 and WIN (read as boolean expression)

str: for all other cases

One question on the __init__ overload would that look like the following:

_PathTypeDefault = unicode if PY2 and WIN else str ... def __init__(self: Path[_PathTypeDefault], ..., path_type: Literal[None] = None): ...

The above conclusion is derived from the following click code:

Special argv parsing for PY2 and WIN, other cases use normal sys.argv which gives us List[str]

https://github.com/pallets/click/blob/1784558ed7c75c65764d2a434bd9cbb206ca939d/src/click/utils.py#L356

if PY2 and WIN and _initial_argv_hash == _hash_py_argv(): return _get_windows_argv() return sys.argv[1:]

with the windows function using windows libs and some custom code to get List[unicode].
https://github.com/pallets/click/blob/1784558ed7c75c65764d2a434bd9cbb206ca939d/src/click/_winconsole.py#L307

For Path options/args eventually one list element will make it to the Path.convert function which in turn calls
coerce_path_result(self, rv) with the value as rv

if self.type is not None and not isinstance(rv, self.type): if self.type is text_type: rv = rv.decode(get_filesystem_encoding()) else: rv = rv.encode(get_filesystem_encoding()) return rv

Now the above is for the 7.1.2 code base 8.0 drops support for python2.7 so the custom code will be removed (i.e., for the pre-release it is already).

@srittau Another side node related the above in the click code review process I noticed that the type hints on the Path functions are not entirely correct.

def coerce_path_result(self, rv: Union[str, bytes]) -> _PathType: ... def __call__(self, value: Optional[str], param: Optional[Parameter] = ..., ctx: Optional[Context] = ...) -> _PathType: ... def convert(self, value: str, param: Optional[Parameter], ctx: Optional[Context]) -> _PathType: ...

I believe the value arg on __call__ and convert should match rv from coerce_path_result.

max-frank · 2020-12-14T09:35:20Z

The tests still seem to be failing with #4815, but I suspect this might be a CI runner cache issue or something since locally the failing test goes through without a problem.

srittau · 2020-12-14T18:18:08Z

@max-frank If you merge master into this branch, the CI failure should be fixed.

max-frank · 2020-12-14T19:20:26Z

@srittau workaround #4821 seems to have done the trick thanks

srittau

One more thing I noticed.

third_party/2and3/click/types.pyi

srittau

I am sorry for the delay. This looks mostly good, but one more thing I noticed below.

srittau · 2020-12-23T11:01:18Z

third_party/2and3/click/types.pyi

+    formats: Iterable[str]
    def __init__(self, formats: Optional[List[str]] = ...) -> None: ...


As len() is called on .formats, this needs to be a Sequence. The List in __init__ could also be changed. Another alternative is to just use List for the attribute to give additional guarantees to users.

I see looking at the doc string for DateTime.__init__ it seems format should be either a Tuple or a List so the current type hint doesnt match this anyway.

So we either replace Iterable[str] and List[str] with Union[List[str], Tuple[str]] or as you suggested use Sequence which would be less strict I guess. Which do you prefer?

Sequence is better I think.

Ok I've updated the PR accordingly

srittau · 2020-12-23T11:03:37Z

third_party/2and3/click/types.pyi

+    mode: str
+    encoding: Optional[str]
+    errors: str
+    lazy: Optional[str]
+    atomic: bool


Side note (not part of your PR): I just noticed that errors and lazy are marked as Optional[] in the __init__() argument, which seems to be wrong.

I did a more thorough check now and it seems my change is wrong for errors while it defaults to the string strict the function calls it is used it actually allows for None to be passed in explicitly.

e.g.,

_make_text_stream

_get_windows_console_stream

Looking at lazy again it seems that the __init__ hint is correct and I made a mistake when reading the code the first time. From the click doc and its usage in the code the attribute should be Optional[bool].

Sorry I should have caught this in my first code review these click functions.
I will be changing the PR to

errors: Optional[str] lazy: Optional[bool]

max-frank

I am sorry for the delay. This looks mostly good, but one more thing I noticed below.

No worries it being so close to Christmas I wasn't expecting this PR to get done this year.
So thanks for your quick reviews.

max-frank · 2020-12-23T13:25:40Z

third_party/2and3/click/types.pyi

+    formats: Iterable[str]
    def __init__(self, formats: Optional[List[str]] = ...) -> None: ...


I see looking at the doc string for DateTime.__init__ it seems format should be either a Tuple or a List so the current type hint doesnt match this anyway.

So we either replace Iterable[str] and List[str] with Union[List[str], Tuple[str]] or as you suggested use Sequence which would be less strict I guess. Which do you prefer?

max-frank · 2020-12-23T13:57:10Z

third_party/2and3/click/types.pyi

+    mode: str
+    encoding: Optional[str]
+    errors: str
+    lazy: Optional[str]
+    atomic: bool


I did a more thorough check now and it seems my change is wrong for errors while it defaults to the string strict the function calls it is used it actually allows for None to be passed in explicitly.

e.g.,

_make_text_stream

_get_windows_console_stream

Looking at lazy again it seems that the __init__ hint is correct and I made a mistake when reading the code the first time. From the click doc and its usage in the code the attribute should be Optional[bool].

Sorry I should have caught this in my first code review these click functions.
I will be changing the PR to

errors: Optional[str] lazy: Optional[bool]

max-frank · 2020-12-23T14:49:56Z

Note I did a finally rebase to be on par with #4825

max-frank commented Dec 13, 2020

View reviewed changes

gvanrossum mentioned this pull request Dec 13, 2020

Fix autogenerated protobuf stub's nonexistent import #4815

Closed

max-frank force-pushed the add-missing-click-types-attributes branch from bc14484 to 4094aab Compare December 14, 2020 09:06

florimondmanca mentioned this pull request Dec 14, 2020

Add table style for option formatting mkdocs/mkdocs-click#25

Merged

max-frank force-pushed the add-missing-click-types-attributes branch from 4094aab to 7339cf3 Compare December 14, 2020 19:07

srittau reviewed Dec 15, 2020

View reviewed changes

third_party/2and3/click/types.pyi Show resolved Hide resolved

max-frank requested a review from srittau December 16, 2020 18:53

srittau requested changes Dec 23, 2020

View reviewed changes

max-frank commented Dec 23, 2020

View reviewed changes

max-frank force-pushed the add-missing-click-types-attributes branch 2 times, most recently from 3a9e8bd to f1d35f7 Compare December 23, 2020 14:36

Add missing attribute hints for click.types

1323c31

max-frank force-pushed the add-missing-click-types-attributes branch from f1d35f7 to 1323c31 Compare December 23, 2020 14:44

srittau approved these changes Dec 23, 2020

View reviewed changes

srittau merged commit 6dae28a into python:master Dec 23, 2020

max-frank deleted the add-missing-click-types-attributes branch December 23, 2020 14:57

		formats: Iterable[str]
		def __init__(self, formats: Optional[List[str]] = ...) -> None: ...

Uh oh!

Add missing attribute hints for click.types #4813

Add missing attribute hints for click.types #4813

Uh oh!

Conversation

max-frank commented Dec 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-frank Dec 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-frank commented Dec 14, 2020

Uh oh!

srittau commented Dec 14, 2020

Uh oh!

max-frank commented Dec 14, 2020

Uh oh!

srittau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

srittau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-frank left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

max-frank commented Dec 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

max-frank Dec 15, 2020 •

edited

Loading