Skip to content

Conversation

@eIGato
Copy link
Contributor

@eIGato eIGato commented Oct 31, 2020

closes #2125.

Based on #2152 with a little fix. closes #2152

@eIGato eIGato force-pushed the type-hinting branch 3 times, most recently from df45d2f to e5a8cf4 Compare October 31, 2020 15:26
@Bibo-Joshi
Copy link
Member

Thanks for the new PR! I edited the PR description, so beth the issue and the old PR will be closed, when merging.
Can you please merge from master? Besides resolving the conflicts that should fix the tests, so we can see better what's going on :)

@eIGato
Copy link
Contributor Author

eIGato commented Oct 31, 2020

Okay. I've just rebased the branch to current master and fixed the conflicts.
I see the monthly job tests are fixed already. I've just started to fix them.
The only thing left is to add tests for new "if-else" branches.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

I had a look at the changes and left 2 comments. Some more general notes:

  • I think it's worth introducing a shortcut type for Union[T, List[T], Tuple[T]] in telegram.utils.types. Maybe STL for single, tuple, list? that should shave some typing for future changes
  • Use of Iterable: see my comment
  • I don't think that this PR should make any changes to non-signature code or tests. Basically the changes in this PR should not cause mypy to be unhappy. Adding tuple type hints where possible is merely a convenience extension of documentation and should not change the actual implementation.

@eIGato
Copy link
Contributor Author

eIGato commented Nov 2, 2020

About changing the implementation. When we started to take Tuple, some calls to external libs became mypy-invalid because some of them require only List. So i converted tuples to lists where possible.
And do you actually think that a list passed to __init__() should ever be preserved as-is when assigned to instance attr? It's understandable that property setters preserve them. But __init__() makes a brand-new instance with brand-new attrs. Preserving lists may lead to bad side-effects that would be hard to find.

Single instance or list/tuple of instances.

Issue: python-telegram-bot#2125
@eIGato
Copy link
Contributor Author

eIGato commented Nov 2, 2020

"STL" sounds too "Std Template Lib", so i chose "SLT".
Pyliint doesn't like T, so i had to use RT.

@Bibo-Joshi
Copy link
Member

About changing the implementation. When we started to take Tuple, some calls to external libs became mypy-invalid because some of them require only List. So i converted tuples to lists where possible.

Frankly, compatibility with 3rd party libs shouldn't be the first priority when making changes. If they need a list, convert the input yourself, that's not PTBs job ;)

And do you actually think that a list passed to __init__() should ever be preserved as-is when assigned to instance attr? It's understandable that property setters preserve them. But __init__() makes a brand-new instance with brand-new attrs. Preserving lists may lead to bad side-effects that would be hard to find.

You have a point here. However, changing

def __init__(self, some_list: List):
    self.some_list = some_list

to

def __init__(self, some_list: List):
    self.some_list = list(some_list)

everywhere in the code is a largely independent issue. As of now the aim of this PR is just to make type hinting a bit more generous. So please either only change signatures or go the full way and adjust the behaviour of every __init__ in the lib ;)

I'll try to have a closer look at the new changes soon.

@eIGato
Copy link
Contributor Author

eIGato commented Nov 2, 2020

MyPy check fails on third-party lib calls inside PTB. If you still can merge such PR, so okay.

or go the full way

You know how to convince people (at least my kind). If you could ignore MyPy or loosen its checks, then say it, and i'll roll the implementation changes back. Even the generator optimization.

@Bibo-Joshi
Copy link
Member

Bibo-Joshi commented Nov 2, 2020

MyPy check fails on third-party lib calls inside PTB. If you still can merge such PR, so okay.

Okay, apparently I misunderstood that. I had understood that mypy becomes invalid when using the changes with some 3rd party libs used in one of your projects. But in this case, I'll resort to "Basically the changes in this PR should not cause mypy to be unhappy. "

@eIGato
Copy link
Contributor Author

eIGato commented Nov 2, 2020

Are there any change requests left?

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Are there any change requests left?

yes. aside from some minor comments, there is still use of Iterable and changes of non-signature code ;)

@eIGato
Copy link
Contributor Author

eIGato commented Nov 4, 2020

The only unnecessary change here was the generator optimization. I've removed it.

Copy link
Member

@Bibo-Joshi Bibo-Joshi left a comment

Choose a reason for hiding this comment

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

Did you check if the :class:`telegram.utils.types.SLT`[:obj:`int`] thingies render correctly when building the docs?

@eIGato
Copy link
Contributor Author

eIGato commented Nov 5, 2020

About rendering thingies. Could not find a way to render them as hyperlinks and maintain readability of the remainder.

@Bibo-Joshi Bibo-Joshi requested a review from Poolitzer November 5, 2020 08:09
@eIGato eIGato requested a review from Bibo-Joshi November 5, 2020 08:25
@Bibo-Joshi Bibo-Joshi removed their request for review November 5, 2020 09:45
@eIGato
Copy link
Contributor Author

eIGato commented Nov 6, 2020

@Poolitzer could you review this PR too?

Copy link
Member

@Poolitzer Poolitzer left a comment

Choose a reason for hiding this comment

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

LGTM

@Bibo-Joshi Bibo-Joshi merged commit 27b03ed into python-telegram-bot:master Nov 7, 2020
@eIGato
Copy link
Contributor Author

eIGato commented Nov 7, 2020

Hi. I'm glad the PRs are merged. But they still don't count for Hacktoberfest for some reason. Could you please add "hacktoberfest-accepted" label to both PRs?

@Bibo-Joshi Bibo-Joshi added the hacktoberfest-accepted other: hacktoberfest-accepted label Nov 7, 2020
@eIGato
Copy link
Contributor Author

eIGato commented Nov 7, 2020

Thank you @Bibo-Joshi. But it didn't work. Hacktoberfest still doesn't count them. May be this is because i submitted these PRs too late (in the evening of 31th of October). Let's pretend this time i contibuted not for the tee)).
image

@github-actions github-actions bot locked and limited conversation to collaborators Nov 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

hacktoberfest-accepted other: hacktoberfest-accepted

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] tuples aren't accepted in static typing

4 participants