Skip to content

Make type-hint tag#1943

Merged
ChrisLovering merged 5 commits into
python-discord:mainfrom
jonathan-d-zhang:type-hint-tag
Apr 3, 2022
Merged

Make type-hint tag#1943
ChrisLovering merged 5 commits into
python-discord:mainfrom
jonathan-d-zhang:type-hint-tag

Conversation

@jonathan-d-zhang
Copy link
Copy Markdown
Contributor

@TizzySaurus

Closes python-discord/meta#154

Let the bikeshedding begin!

@jonathan-d-zhang jonathan-d-zhang marked this pull request as ready for review November 8, 2021 23:31
@TizzySaurus TizzySaurus added a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: tags Related to bot tags s: needs review Author is waiting for someone to review and approve review: do not merge The PR can be reviewed but cannot be merged now p: 3 - low Low Priority labels Nov 8, 2021
@TizzySaurus
Copy link
Copy Markdown
Contributor

TizzySaurus commented Nov 8, 2021

@SavagePastaMan

A few first-glance alterations:

I don't like that we're calling the variable sum, thus overriding the built-in sum() function. The type hint here also doesn't particularly make sense. Inline assignment type hints like this should only be done when the type isn't obvious from the variables / other type hints (the removal of the type hint arguably makes it better to just directly return rather than first assigning to a variable).

There's also some small rewording I've done here.

Current proposal:
image

We should probably link to some further resources on type hints (perhaps a PEP?) and also mention static type checkers like mypy which can be used to "enforce" type hints.

I'm going to bed now, will look further into this tomorrow 👍

*(Apparently my tired brain forgot to add the labels all at once so sorry for the spam there.)

@jonathan-d-zhang
Copy link
Copy Markdown
Contributor Author

jonathan-d-zhang commented Nov 8, 2021

The type hint here also doesn't particularly make sense.

I wanted to include it to show that you can typehints normal variables too, maybe we can find another place to show that? I agree with not naming it sum though.

We should probably link to some further resources on type hints (perhaps a PEP?) and also mention static type checkers like mypy which can be used to "enforce" type hints.

Yes, I agree.

Also,

Both parameters and the return type of the function is said to be integers.

Should be "... function are said to ..."

@TizzySaurus

Copy link
Copy Markdown
Contributor

@Bluenix2 Bluenix2 left a comment

Choose a reason for hiding this comment

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

🚴‍♂️🚴‍♂️🚴‍♂️💨💨

Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Added links to mypy and PEP484 (the type hint pep).
Removed the temp variable for clarity, maybe it's fine if we don't show that you can typehint normal variables?
Copy link
Copy Markdown
Contributor

@TizzySaurus TizzySaurus left a comment

Choose a reason for hiding this comment

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

Few minor things

Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
@Bluenix2 Bluenix2 added s: waiting for author Waiting for author to address a review or respond to a comment and removed s: needs review Author is waiting for someone to review and approve labels Nov 28, 2021
Copy link
Copy Markdown
Contributor

@onerandomusername onerandomusername left a comment

Choose a reason for hiding this comment

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

Given that type-hinting also exists, it may be worth mentioning that pydis has that channel as well? Not sure.

Comment thread bot/resources/tags/type-hint.md Outdated
Comment thread bot/resources/tags/type-hint.md Outdated
@wookie184
Copy link
Copy Markdown
Contributor

@jonathan-d-zhang are you interested in continuing this? If not i'd be happy to take over.

@jonathan-d-zhang
Copy link
Copy Markdown
Contributor Author

@wookie184 oops, I kinda forgot about this yeah, you can take over if you'd like

@wookie184 wookie184 self-assigned this Mar 31, 2022
@wookie184 wookie184 requested a review from ichard26 March 31, 2022 17:51
@wookie184
Copy link
Copy Markdown
Contributor

I've made a couple of changes, feel free to request changes on any of them if you think anything should be reverted to previous wording or changed further

@wookie184 wookie184 added s: needs review Author is waiting for someone to review and approve and removed s: waiting for author Waiting for author to address a review or respond to a comment review: do not merge The PR can be reviewed but cannot be merged now labels Apr 1, 2022
Comment thread bot/resources/tags/type-hint.md Outdated
Co-authored-by: ChrisJL <ChrisLovering@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

STGM.

@ChrisLovering ChrisLovering enabled auto-merge April 3, 2022 18:30
@ChrisLovering ChrisLovering merged commit f0bfa41 into python-discord:main Apr 3, 2022
@Xithrius Xithrius removed the s: needs review Author is waiting for someone to review and approve label May 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: backend Related to internal functionality and utilities (error_handler, logging, security, utils and core) a: tags Related to bot tags p: 3 - low Low Priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add "What's a typehint" tag

10 participants