Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Type hinting#1920

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
Bibo-Joshi merged 54 commits intov13fromtype_hinting_master
Oct 6, 2020
Merged

Type hinting#1920

Bibo-Joshi merged 54 commits intov13fromtype_hinting_master
Oct 6, 2020

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-JoshiBibo-Joshi commentedApr 25, 2020•
edited
Loading

Preparations for type hinting:

  • Mypy as static type checker is added to pre-commit and make-file

  • Python 3.5 support is dropped, to allow typing for variables

  • Two make sure that new PRs include typing, I tried two approaches:

    1. Use mypys report functionality together withdiff-cover to compute the coverage of the changed lines only and let the test fail, if its < 100%. Unfortunately, mypy will mark any line with an "Any" as uncovered, even if that's allowed during type checking. But especially intelegram.ext we have a lot of stuff that can be "Any" (CH states, entries ofuser_data and so on). So, even after adding typing for the current code base, we would either
      1. have failing tests all the time or
      2. Allow the test to fail and manually look at the logs. grrr.
    2. Use mypys settings to disallow function definitions with no or incomplete typing. Upside compared to thediff-cover approach: its easier to see, if we're satisfied or not. Downside: When a file is changedall functions in that file will be checked andall classes/functions that are imported or inherited from, too. We can either
      1. type the complete base before merging
      2. try to single out the mypy result from thepre-commit test and allow it to fail until the whole base is typed
      3. Just live with the fact the the first few PRs after merging will either fail the pre-commit test or will have to do a lot of type hinting

    Personally, I like ii. better that i., which is why this is the current state of this pr. More precisely, I think I'd vote for ii.c)

Closes#1373

@Bibo-JoshiBibo-Joshi added enhancement ⚙️ documentationaffected functionality: documentation labelsApr 25, 2020
@Bibo-JoshiBibo-Joshi changed the titleType hinting masterType hintingApr 25, 2020
@Bibo-Joshi
Copy link
MemberAuthor

Bibo-Joshi commentedMay 16, 2020•
edited
Loading

Okay, I prepared for going for ii.a) :D

TBH, I didn't try to be as specific as possible. I rather tried to annotate most almost everything on a reasonably specific level and made sure that Mypy checks out. Many things can surely be improved, but IMO they can be revisited if

  1. We're touching the code anyway
  2. People have issues with the hints I wrote,

i.e. we can gradually improve the hints.

Some more specific notes:

  • Updated flake8 so it doesn't complain about# type: ignore[error-code]

  • Telegram classes that have class methods using the bot, would need aif self.bot is None: raise RuntimeError or something, if we're strict. I found that unnecessary and just told mypy to not check optionals strictly in those classes (seesetup.cfg)

  • Not usingLiterals just yet as this is the first iteration on typing and they're natively supported only for py>=3.8. They could be useful for things likePoll.type where only specific strings are allowed

  • NoTypedDicts yet as this is the first iteration. Thesecould be used forto_dict,de_json. However the need is not too big, as those are internal methods

  • reduced code duplication ofde_list on the fly

  • Noticed thetMessage.from_user was a positional argument, which is wrong. Fixed. This needs a Release note, as this might be breaking. On the other hand, manually instantiatingMessages doesn't happen too often, mostly in tests …

  • I put sometype: ignore where I didn't think doing it 100% correct would be worth the trouble

  • Request.stop(): Item "AppEngineManager" of "Union[PoolManager, AppEngineManager, SOCKSProxyManager, ProxyManager]" has no attribute "clear" is that a bug?

  • Passports are scary. I scipped some typing there

  • Values returned by requests are kind of a trust thingy. I'm ignoring all return value warnings from there right now as we kind of have to expect Telegram to return what they are promising.

  • Handlers: callback is typed asupdate, context as old api is deprecated

  • Similar for Job callbacks

  • telegram.ext is a bit harder to type for several reasons:

    • We're doing some hacky stuff, e.g. callingFilters.filter withUpdateorMessage
    • Handlers are a bit tricky, as theyusually acceptUpdates but for advanced handlers,str or evenobject (forTypeHandler) are okay
    • The docs are usually written w.r.t. to handlingUpdates, partly ignoring the other cases
    • The internals ofUpdater are a black box for me. I just marked a bunch of functions os "no type check" here

    That's why I've been a bit more lax ontelegram.ext. Big parts of the typing usually isn't relevant for the user most times, anyway …

Comments on CI:

  • Codacy is complaining about unused imports, which are needed for typing.
  • Codecov (only gave a quick look) seems to mainly complain about theif TYPE_CHECKING not being covered. We can't do anything against that, i fear …

@Bibo-JoshiBibo-Joshi added this to the13.0 milestoneJun 12, 2020
# Conflicts:#telegram/__main__.py#telegram/bot.py#telegram/callbackquery.py#telegram/chatmember.py#telegram/choseninlineresult.py#telegram/error.py#telegram/ext/callbackqueryhandler.py#telegram/ext/commandhandler.py#telegram/ext/conversationhandler.py#telegram/ext/dictpersistence.py#telegram/ext/filters.py#telegram/ext/inlinequeryhandler.py#telegram/ext/jobqueue.py#telegram/ext/messagequeue.py#telegram/ext/picklepersistence.py#telegram/ext/regexhandler.py#telegram/ext/stringcommandhandler.py#telegram/ext/stringregexhandler.py#telegram/ext/typehandler.py#telegram/files/animation.py#telegram/files/document.py#telegram/files/sticker.py#telegram/files/venue.py#telegram/files/video.py#telegram/files/videonote.py#telegram/files/voice.py#telegram/games/game.py#telegram/games/gamehighscore.py#telegram/inline/inlinekeyboardmarkup.py#telegram/inline/inlinequery.py#telegram/message.py#telegram/messageentity.py#telegram/passport/credentials.py#telegram/passport/encryptedpassportelement.py#telegram/passport/passportdata.py#telegram/passport/passportfile.py#telegram/payment/orderinfo.py#telegram/payment/precheckoutquery.py#telegram/payment/shippingoption.py#telegram/payment/shippingquery.py#telegram/payment/successfulpayment.py#telegram/poll.py#telegram/replykeyboardmarkup.py#telegram/update.py#telegram/user.py#telegram/userprofilephotos.py#telegram/utils/helpers.py#telegram/utils/webhookhandler.py
# Conflicts:#telegram/bot.py#telegram/botcommand.py#telegram/callbackquery.py#telegram/chat.py#telegram/chatpermissions.py#telegram/dice.py#telegram/ext/basepersistence.py#telegram/ext/conversationhandler.py#telegram/ext/dictpersistence.py#telegram/ext/dispatcher.py#telegram/ext/filters.py#telegram/ext/jobqueue.py#telegram/ext/messagequeue.py#telegram/ext/picklepersistence.py#telegram/ext/updater.py#telegram/files/animation.py#telegram/files/audio.py#telegram/files/document.py#telegram/files/photosize.py#telegram/files/sticker.py#telegram/files/video.py#telegram/files/videonote.py#telegram/files/voice.py#telegram/games/game.py#telegram/inline/inlinekeyboardbutton.py#telegram/inline/inlinekeyboardmarkup.py#telegram/inline/inputlocationmessagecontent.py#telegram/message.py#telegram/parsemode.py#telegram/passport/passportfile.py#telegram/payment/invoice.py#telegram/poll.py#telegram/replykeyboardmarkup.py#telegram/update.py#telegram/userprofilephotos.py#telegram/utils/helpers.py#telegram/utils/request.py#telegram/utils/webhookhandler.py#telegram/webhookinfo.py#tests/test_conversationhandler.py#tests/test_message.py#tests/test_updater.py
@Bibo-JoshiBibo-Joshiforce-pushed thev13 branch 2 times, most recently from3247477 tob7293a4CompareSeptember 27, 2020 15:32
# Conflicts:#README.rst#telegram/bot.py#telegram/botcommand.py#telegram/chatmember.py#telegram/chatpermissions.py#telegram/dice.py#telegram/ext/basepersistence.py#telegram/ext/defaults.py#telegram/ext/dispatcher.py#telegram/ext/filters.py#telegram/ext/jobqueue.py#telegram/ext/updater.py#telegram/files/animation.py#telegram/files/audio.py#telegram/files/document.py#telegram/files/file.py#telegram/files/photosize.py#telegram/files/sticker.py#telegram/files/video.py#telegram/files/videonote.py#telegram/files/voice.py#telegram/games/game.py#telegram/inline/inlinekeyboardbutton.py#telegram/inline/inlinekeyboardmarkup.py#telegram/inline/inlinequery.py#telegram/message.py#telegram/passport/passportfile.py#telegram/payment/invoice.py#telegram/poll.py#telegram/replykeyboardmarkup.py#telegram/update.py#telegram/userprofilephotos.py#telegram/utils/helpers.py#telegram/utils/request.py#telegram/utils/webhookhandler.py#telegram/webhookinfo.py#tests/test_jobqueue.py#tests/test_message.py
@@ -144,7 +149,8 @@ def edit_message_text(self, text, *args, **kwargs):
return self.bot.edit_message_text(text, chat_id=self.message.chat_id,
message_id=self.message.message_id, *args, **kwargs)

def edit_message_caption(self, caption, *args, **kwargs):
def edit_message_caption(self, caption: str, *args: Any,
**kwargs: Any) -> Union[Message, bool]:
Copy link
Contributor

Choose a reason for hiding this comment

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

strange line break

@@ -172,7 +178,8 @@ def edit_message_caption(self, caption, *args, **kwargs):
message_id=self.message.message_id,
*args, **kwargs)

def edit_message_reply_markup(self, reply_markup, *args, **kwargs):
def edit_message_reply_markup(self, reply_markup: 'InlineKeyboardMarkup', *args: Any,
Copy link
Contributor

@JosXaJosXaOct 2, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Tbh I'd just add black to pre-commit.yml aswell if every single file is being changed anyway...

-   repo: https://github.com/psf/black    rev: 20.8b1    hooks:    -   id: black

Advantage: Never ever any discussion around formatting again.
Disadvantages: None. Except you don't like PEP8 and prefer your own code style to keep your job with this Arbeitsbeschaffungsmaßnahme. ;D

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I think code formatting is worth its own discussion and PR

Copy link
Member

@PoolitzerPoolitzer left a comment

Choose a reason for hiding this comment

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

Monumental PR! I eagerly look forward to have static typing supported in our library. Just a couple of small discussion points, most of them repeat themself. Then I am all good with it. Thank you so much for putting the time into it.

Copy link
Member

@PoolitzerPoolitzer left a comment

Choose a reason for hiding this comment

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

Looks good to me now! If there actually are errors, we can fix them in a quick release after 13. I would merge.

@Bibo-Joshi
Copy link
MemberAuthor

Codecov is still failing, but that's mostly due to this PR

  • annotating code that wasn't tested before
  • adding someif-clauses that are just there to make mypy happy

So we can just up the coverage the time we change some code … Merging :)

(Codacy fails, because the v13 branch is not tracked …)

@Bibo-JoshiBibo-Joshi merged commitd2e67d1 intov13Oct 6, 2020
@Bibo-JoshiBibo-Joshi deleted the type_hinting_master branchOctober 6, 2020 16:05
@Bibo-JoshiBibo-Joshi mentioned this pull requestOct 6, 2020
Bibo-Joshi added a commit that referenced this pull requestOct 6, 2020
# Conflicts:#telegram/error.py#telegram/ext/callbackcontext.py#telegram/ext/callbackqueryhandler.py#telegram/ext/commandhandler.py#telegram/ext/conversationhandler.py#telegram/ext/dispatcher.py#telegram/ext/handler.py#telegram/ext/inlinequeryhandler.py#telegram/ext/messagehandler.py#telegram/ext/regexhandler.py#telegram/ext/stringcommandhandler.py#telegram/ext/stringregexhandler.py#telegram/ext/typehandler.py#telegram/utils/promise.py
Bibo-Joshi added a commit that referenced this pull requestOct 6, 2020
# Conflicts:#telegram/error.py#telegram/ext/callbackcontext.py#telegram/ext/callbackqueryhandler.py#telegram/ext/commandhandler.py#telegram/ext/conversationhandler.py#telegram/ext/dispatcher.py#telegram/ext/handler.py#telegram/ext/inlinequeryhandler.py#telegram/ext/messagehandler.py#telegram/ext/regexhandler.py#telegram/ext/stringcommandhandler.py#telegram/ext/stringregexhandler.py#telegram/ext/typehandler.py#telegram/utils/promise.py
Bibo-Joshi added a commit that referenced this pull requestOct 7, 2020
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsOct 7, 2020
@Bibo-JoshiBibo-Joshi added 🔌 enhancementpr description: enhancement and removed enhancement labelsNov 3, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@septatrixseptatrixseptatrix left review comments

@JosXaJosXaJosXa approved these changes

@PoolitzerPoolitzerPoolitzer approved these changes

Assignees
No one assigned
Labels
⚙️ documentationaffected functionality: documentation🔌 enhancementpr description: enhancement
Projects
None yet
Milestone
13.0
Development

Successfully merging this pull request may close these issues.

Type-hinting for IDEs
4 participants
@Bibo-Joshi@JosXa@septatrix@Poolitzer
[8]ページ先頭
©2009-2025 Movatter.jp