search

tkrajina / gpxpy

gpx-py is a python GPX parser. GPX (GPS eXchange Format) is an XML based file format for GPS tracks.
Apache License 2.0
1.02k stars 223 forks source link

Type hints #185

Closed constantin-schuett closed 4 years ago

constantin-schuett commented 4 years ago

Hello,

I would like contribute type hints to make it even easier to use this module (mostly for geo.py). Before starting I just wanted to make sure that I didn't overlook existing type hints and also wanted to ask if this is something you would consider pulling?

As I understand this module is intended for Python 2.7 as well as 3. Therefore I would put type hints either into comments or docstrings.

tkrajina commented 4 years ago

Hey, thanks for your offer, but - just yesterday I pushed typing support. It's not yet in master, but you can see it here: https://github.com/tkrajina/gpxpy/commit/81cb1eb0c3f50b0fd771ef12f0276fd92b3885a6 (in dev).

This is the first time I'm implementing type hints on a python project - feel free to go through the code. Suggestions/ideas/bugfixes are more than welcome.

Ah, and regarding python 2.7. Since 2.7 EOL is in a few weeks I decided to (finally!!!) drop python 2.7 support. Once I merge dev into master, the library will be for py 3.6+.

constantin-schuett commented 4 years ago

Aaaah that's great news, I love type hints in python 3!

My biggest suggestion: Instead of importing the whole typing module I would just import the needed parts. This would shorten all those "typing.Optional[]" to just "Optional". I made an example here: 55ace23.

Other small thing I noticed: In gpxpy you are using cast once to resolve a mypy error and use "# type: ignore" in all other cases. I would just stick to the comment version as I find it easier to read (also in my example commit).

I hope you can access this, if not please let me know (I am new to github).

tkrajina commented 4 years ago

Yes, you're right about importing.

Also, yeah, I'm probably not using type: ignore and casts consistently. Will try to fix both suggestions before I merge this in master.