Skip to content

Add more type hints #397

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 8, 2022
Merged

Add more type hints #397

merged 3 commits into from
Aug 8, 2022

Conversation

alexkahan
Copy link
Contributor

Add type hints to multiple design pattern examples

Related to #373

Copy link

@JKearnsl JKearnsl left a comment

Choose a reason for hiding this comment

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

Using the "-> None" hint in init is meaningless, since init always returns None and we can't use that anywhere.

The same goes for "-> str" in repr and str as we already deliberately know that these methods always return a string -> this is pointless

@alexkahan
Copy link
Contributor Author

Using the "-> None" hint in init is meaningless, since init always returns None and we can't use that anywhere.

The same goes for "-> str" in repr and str as we already deliberately know that these methods always return a string -> this is pointless

@JKearnsl i thought i was following pep 484 guidelines as well as what is found throughout the rest of the repo. __init__() is receiving the None type hint throughout. happy to remove that and the __repr__ type hint though, if that is preferred (there is another __repr__ type hinted like this in another file, so i'll remove that one too).

@faif
Copy link
Owner

faif commented Jul 29, 2022

We have already discussed about the None hints in another issue, and agreed to keep them, since this seems to be the convention. I'm also not a big fan of them, but I won't go against the syntax conventions of type hints.

@JKearnsl
Copy link

We have already discussed about the None hints in another issue, and agreed to keep them, since this seems to be the convention. I'm also not a big fan of them, but I won't go against the syntax conventions of type hints.

OK.
I'm just going from the logical meaning of the clues, not from mindless beliefs.

@faif
Copy link
Owner

faif commented Aug 7, 2022

Hi @alexkahan, can you look into the linter issues? Thanks.

@alexkahan
Copy link
Contributor Author

Hi @alexkahan, can you look into the linter issues? Thanks.

Sorry for the delay. Should be fixed, seemed like it was having an issue with the List type hint

@faif faif merged commit cc54961 into faif:master Aug 8, 2022
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.

3 participants