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

Initial Typing Evaluation #99

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

Cabalist
Copy link
Contributor

@Cabalist Cabalist commented Apr 4, 2018

Hey there!

This is a first pass at MyPy style typing. The goal of this is too include structured typing to make the migration to Python3 easier and catch some bugs. The Hershey extension was small enough and self contained so was a good candidate for this.

In an effort to get the typing correct I reformatted the hersheydata.py file. It is now considerably longer but not considerably larger. (361 lines -> 5301 lines , 1912 bytes -> 2048 bytes) I changed the final font groupings to a list of tuples instead of a list of strings since the data seemed immutable in the code and I could make a stronger type declaration on tuples.

This typing exercise caught no issues. Haha. BUT it does show what I am hoping to accomplish with some of the more complex code. Take a look and let me know your thoughts.

P.S. I saw your posts in the Inkscape mailing lists contributing Python3 compatibility. It looks like some progress has been made on that in their Gitlab repo and they are pushing toward a 1.0 release.

@oskay
Copy link
Contributor

oskay commented Apr 5, 2018

This will take a little more time to look at-- I have some other code that relies upon hersheydata.py, and will likely break.

@Cabalist
Copy link
Contributor Author

Cabalist commented Apr 5, 2018

Changes to hersheydata.py are entirely whitespace except for the list -> tuple change. The only thing that should break is if you have code that changes a font name in place.

I really am trying to reduce the size of these pull requests I swear!

@oskay
Copy link
Contributor

oskay commented Apr 5, 2018

So far this isn't breaking anything, but this is quite a deep change -- to include another library that isn't supported across the platforms that we support and now to require that comments are parsed in addition to the whitespace.

@Cabalist
Copy link
Contributor Author

Cabalist commented Apr 5, 2018

I hear you. I am not planning to add dependencies to the inkscape plugins. My motivation is primarily to ease the transition of math heavy code to Python3 and hopefully remove some otherwise unspotted bugs.

To your concern:

  1. typing is not active during runtime. At all. Think of them like docstrings.

  2. if the user doesn't have the typing module installed (and they probably won't) it isn't imported and since the type hints are all comments nothing changes for the user.

  3. The types are added purely to assist in development and in coordination with an IDE, testing suite (via Pytest-mypy) or automated tools like Travis CI. If something 'fails' a type check it does not change the code's ability to run.

This is not typing in the vein of C or even Typescript this is just type hints. :) This is still vanilla python.

@oskay
Copy link
Contributor

oskay commented Apr 5, 2018

I understand these three things intellectually. It will take me a bit to read through the relevant PEPs and get comfortable with the idea.

@Cabalist
Copy link
Contributor Author

Cabalist commented Apr 5, 2018

No rush. :)

@Cabalist
Copy link
Contributor Author

Cabalist commented May 6, 2018

Just poking my head in again and bumping this topic. :)

@oskay
Copy link
Contributor

oskay commented May 7, 2018

I have spent a few hours on it, but I'm still not up to speed. This is a rabbit hole.

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