rndusr / torf

Python module to create, parse and edit torrent files and magnet links
GNU General Public License v3.0
180 stars 17 forks source link

add type hints #47

Closed Ravencentric closed 1 month ago

Ravencentric commented 1 month ago

This PR adds type hints to the public API of torf. I've tried my best to get the types right with the help of documentation and plain old testing. For this PR, I've chosen to seperate the hints from the implementation but if you would prefer it be moved to implementation, I'm happy to do so.

References:

Ravencentric commented 1 month ago

That was a misclick

Ravencentric commented 1 month ago

\cc @rndusr I would also like your opinion on this PR!

rndusr commented 1 month ago

Sorry, I haven't forgotten about this. (Yet. :P)

I have never really used type hints, so my opinion is probably less important than yours.

I just need to find the time to go through this and see if it makes sense to me.

rndusr commented 1 month ago

OK, mypy . is green so this must be good. :) (As I said, I have no experience with type hints.)

The only thing I don't like much is the introduction of __all__. To me, this has always been just another thing that increases my janitor workload and nibbles at my code-writing time. Prefixing private stuff with "_" works just as well in my experience, and I can immediately see if something is meant to be private or public.

Is __all__ necessary for the type hints or why did you add that?

Ravencentric commented 1 month ago

Is all necessary for the type hints or why did you add that?

Nope! It can be left out. It's only affects star imports which already shouldn't matter. I only added it for completeness' sake.

Ravencentric commented 1 month ago

OK, mypy . is green so this must be good. :) (As I said, I have no experience with type hints.)

I really appreciate you merging this despite that! I should be able help if any future complaints regarding the typehints arise. They are really nice for the end user experience since it tells them what type of input a function is expecting without going through the docs and most modern IDEs like vscode and pycharm provide autocompletion and error reporting based on them.

rndusr commented 1 month ago

OK, I've removed __all__ for the reasons I explained above.

To be honest, I was close-ish to dismissing this PR, but you gotta move out of your comfort zone sometimes. Who knows, maybe I will see the light eventually.

Thanks again for your work! I'm sure there are users out there who appreciate it more than I do.

Ravencentric commented 1 month ago

I'm sure there are users out there who appreciate it more than I do.

Me! I already have a couple projects that depend on torf which is what motivated me to make this PR 😄