Skip to content

Conversation

@cs-shadow
Copy link
Contributor

@cs-shadow cs-shadow commented Jun 1, 2019

Closes #3020.


This is mostly based on differences I noticed in git diff, not sure if there's an easy way to verify that I've got them all.

Also, this is my first PR here so please let me konw if I've missed something 😄

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

Thanks! Just a few comments.



def echo_via_pager(text: str, color: Optional[bool] = ...) -> None:
def echo_via_pager(text_or_generator: Union[str, Iterable[str]], color: Optional[bool]) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

The argument can also be a generator function, something like Callable[[], Generator[str, None, None]] (https://github.com/pallets/click/blob/master/click/termui.py#L246).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, missed that at first. Should be good now.

(I am assuming using Iterable[str] in place of Generator[str, None, None] is fine).

Copy link
Member

Choose a reason for hiding this comment

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

I do think it should be Generator, because the check specifically uses isgeneratorfunction. lambda: ["a", "b"] would not work at runtime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right! Updated it now.

exc_info: Optional[Any]
stdout_bytes: bytes
stderr_bytes: bytes
# properties
Copy link
Member

Choose a reason for hiding this comment

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

Should be given with @property. It matters because this way type checkers can see that assigning to .output is an error.

Copy link
Contributor Author

@cs-shadow cs-shadow Jun 2, 2019

Choose a reason for hiding this comment

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

Thanks for the tip, I didn't consider that.

I have fixed it here but I also noticed this pattern in a few other places, like in parser.py and core.py. I wonder if they too need fixing or if I am missing some subtle detail?

Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked these against the implementation, but if those are also @propertys without setters, they should use @property in the stub. (If they do have a setter, it's fine to just represent them in the stub as you did here.)

Copy link
Contributor Author

@cs-shadow cs-shadow Jun 3, 2019

Choose a reason for hiding this comment

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

Thanks! I guessed so, but wanted to double check.

I verified that those properties do not have setters, and submitted #3028 to fix that.

cs-shadow added a commit to cs-shadow/typeshed that referenced this pull request Jun 3, 2019
Since these properties do not have a setter, be explicit with the
`@property` decorator. This will allow type checkers to see that
assignment of these attributes is an error.

See python#3027 (comment)
for some related context.
cs-shadow added a commit to cs-shadow/typeshed that referenced this pull request Jun 3, 2019
Since these properties do not have a setter, be explicit with the
`@property` decorator. This will allow type checkers to see that
assignment of these attributes is an error.

See python#3027 (comment)
for some related context.
cs-shadow added a commit to cs-shadow/typeshed that referenced this pull request Jun 3, 2019
Since these properties do not have a setter, be explicit with the
`@property` decorator. This will allow type checkers to see that
assignment of these attributes is an error.

See python#3027 (comment)
for some related context.
@JelleZijlstra JelleZijlstra merged commit 018ecb3 into python:master Jun 3, 2019
JelleZijlstra pushed a commit that referenced this pull request Jun 3, 2019
Since these properties do not have a setter, be explicit with the
`@property` decorator. This will allow type checkers to see that
assignment of these attributes is an error.

See #3027 (comment)
for some related context.
@cs-shadow cs-shadow deleted the click-7-stubs branch June 24, 2019 21:44
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.

Update type hints for click 7.x?

2 participants