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 type annotations everywhere #43

Open
tgpfeiffer opened this issue Apr 20, 2020 · 6 comments
Open

Add type annotations everywhere #43

tgpfeiffer opened this issue Apr 20, 2020 · 6 comments
Assignees

Comments

@tgpfeiffer
Copy link

While researching about treating measurement data in a type-safe way in Python (like squants for Scala), I discovered this library, which seems very close to providing such a function. However, I think there are no type annotations yet.

I would like to contribute in that area, so where is a good place to do that? It seems that you are about to release a new major version, so probably it makes more sense to start on the latest master than on the 3.2 branch, right?

@codingjoe
Copy link
Collaborator

Hi @tgpfeiffer,

Thanks for reaching out. So happy to see, that people interested in this package. BTW, you don't happen to be Tobias Pfeffer – HPI class of 2011 – which whom I studied together, are you?

Anyhow, regarding your question:
I didn't know squants before, but it's similar. However, this package, at least in its alpha, version is slightly more sophisticated. For starters, it's more accurate, but we also support all metric prefixes and more units. Beyond that, we also support arithmetic operations across types, for geometries and electromagnetism.

Regarding your typing question. Version 4 comes with type hints. However, maybe not all places are annotated yet. We could consider adding mypy to lint type annotations. In any event, yes, please only contribute to v4. I am hoping to release it pretty soon, so I'd appreciate another set of eyes very much.

I am curious though, why are you so eager to have type hints?

Best
-Joe

@tgpfeiffer
Copy link
Author

BTW, you don't happen to be Tobias Pfeffer – HPI class of 2011 – which whom I studied together, are you?

No, that's not me ;-) (I'm also from Berlin, though.)

I am curious though, why are you so eager to have type hints?

I think it'd be a killer feature for this library if you can use it in type-annotated Python code and can encode the data that you are working with in your function signatures, or in dataclasses that you are passing around.

In any event, yes, please only contribute to v4.

OK, cool, let me have a look at the code and see what I can do :-)

@codingjoe
Copy link
Collaborator

Sounds good, I will keep this ticket open to track how we are doing on complete type hinting support.

@codingjoe codingjoe changed the title Type annotations Add type annotations everywhere Apr 21, 2020
@tgpfeiffer
Copy link
Author

Please see master...tgpfeiffer:43-type-ann for my additions. I have not sent a PR yet, because it is WIP and I wanted to discuss some points with you before.

I have added some fairly strict mypy settings to the setup.cfg file and, just so that we can discuss changes, a mypy.log file with the latest mypy . output, which should be absent from the final PR.

From here, there are three major issues as far as I can see.

  1. Some class internals (in particular metaclass-related) I found a bit too complicated to get into, and sometimes I could not find out where a certain member actually belongs to, e.g.

    • measurement/base.py:225: error: "type" has no attribute "_units"
    • measurement/base.py:302: error: "AbstractUnit" has no attribute "org_name"

    I think here is some annotation work for you ;-), as you probably have the best understanding of class internals.

  2. The majority of the remaining error messages is where we have a Union or Optional, but not all of the possible values are covered. We have initializations like name = None (then the type is inferred as None) or name: str = None (which is wrong, because None is not a str), where it should be name: Optional[str] = None (and then we need to deal with all None cases, which is annoying), or just write name: str (and then make 100% sure we assign a value later). Also, some cleanup work is necessary around Union[str, Decimal] or Union[str, Decimal, int, None] values.

  3. Here is a difficult one. The annotation for __add__() was simple, because in all subclasses this method always behaves the same. However, for __mul__() we cannot do a similar annotation because (1) some subclasses have larger definition ranges than the AbstractMeasure (e.g. Frequency can be multiplied with a Time), (2) so we need to @overload-annotate the subclass, but (3) this is not possible with special methods, see Signatures of overloaded operators are incompatible with super type python/mypy#4985 (comment). It's not so clear how to deal with this if you want to keep the pythonic a * b notation...

Please let me know any thoughts you have, and what you think is a good way to proceed.

@codingjoe
Copy link
Collaborator

Would you mind opening a PR in draft mode? That's what they are for, and they will make it easier for me to comment on your questions as well as on your code. Best, Joe

@tgpfeiffer
Copy link
Author

@codingjoe Did you see my draft PR at #47?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants