Skip to content
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

Add PEP484 type hints #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stickies-v
Copy link
Contributor

I find PEP484 type hints make reading and debugging significantly easier for non-trivial functions. As I went through the codebase I added type hints everywhere, initially just for my understanding but then I thought you might like to have it here too so I went for full coverage.

As there is some maintenance burden I'd understand if you prefer not to merge it, it's personal preference. It's a lot of LoC changed, but type hints do not affect program behaviour so I think this can be pretty low-touch review (a wrong type hint can always be fixed later). Happy to split this up in smaller commits too if that makes it easier.

@stickies-v stickies-v force-pushed the add-pep484-type-hints branch 2 times, most recently from f0e63b3 to ea8f460 Compare May 5, 2022 10:18
@stickies-v
Copy link
Contributor Author

Rebased to fix merge conflicts

@darosior
Copy link
Owner

darosior commented May 5, 2022

Thanks, just to let you know: i saw this PR, i'm a bit on the fence about it. I'll think about it.

Type hints make reading and debugging significantly easier.
See https://peps.python.org/pep-0484/
@stickies-v stickies-v force-pushed the add-pep484-type-hints branch from ea8f460 to 936ca48 Compare May 5, 2022 16:17
@stickies-v
Copy link
Contributor Author

Yup no problem. I've just force pushed an update to directly import the used types to improve readability (at cost of naming collision, but the trade-off seems worthwhile here).
So instead of typing.Optional[typing.List[bytes]], it's now just Optional[List[bytes]]

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.

2 participants