Add type hints

[kasium]

Requirement - what kind of business use case are you trying to solve?

As you might know many python3 only packages already have type hints present based on the typing module. Afterwards the types can be checked e.g. with mypy. It would be great if you could add type hints and mark the package as types (aka add a py.typed file)

Problem - what in Jaeger blocks you from solving the requirement?

Missing type hints in the code base

Proposal - what do you suggest to solve the problem or improve the existing situation?

Add type hints 😄

[kasium]

@yurishkuro I'm happy to give a contribution a try. However, I guess with the current support python versions it will get hard. I would recommend to drop at least python 3.5 support if not even 3.6. What do you think?

[yurishkuro]

yes, it's fine to raise min supported version (in separate PR)

[kasium]

So, should I just change the setup.py file or also other stuff?

[yurishkuro]

Also GitHub actions

[kasium]

@yurishkuro so I see the following needed types

• reporter ( Add types to reporter #333 )
• config ( Add types to config #334 )
• util ( Add basic typing support #326 )
• tracer ( Add typing to tracer.py #328 )
• span ( Add types for span, metrics, throttler #336 )
• metircs ( Add types for span, metrics, throttler #336 )
• throttler ( Add types for span, metrics, throttler #336 )
• sampler ( Add sampler types and py.typed file #338 )
• py.typed file ( Add sampler types and py.typed file #338 )

Can you please check if something is missing or is too much

[yurishkuro]

yep, looks just right

[kasium]

@yurishkuro I currently hit an issue with my latest types (local change)

ImportError while loading conftest '/root/git/jaeger-client-python/tests/conftest.py'.
tests/conftest.py:17: in <module>
    from jaeger_client import ConstSampler, Tracer
jaeger_client/__init__.py:18: in <module>
    from .tracer import Tracer  # noqa
jaeger_client/tracer.py:35: in <module>
    from .span import Span, SAMPLED_FLAG, DEBUG_FLAG
jaeger_client/span.py:26: in <module>
    from .tracer import Tracer
E   ImportError: cannot import name 'Tracer' from 'jaeger_client.tracer' (/root/git/jaeger-client-python/jaeger_client/tracer.py)

This happens because tracer.py imports span.py, but span.py needs tracer.py to type the constructor. So I would need the if TYPE_CHECKING construct which will decrease the coverage

[yurishkuro]

Does mypy have any means of adding types without explicit imports?

If no other solution, we'd have to take a hit on the coverage.

[kasium]

Doesn't look like it's supported: https://mypy.readthedocs.io/en/latest/common_issues.html#import-cycles

[yurishkuro]

I am curious why it works when TYPE_CHECKING is true, doesn't it still create a circular import?

Does mypy have any means of adding types without explicit imports?

what I meant was if there is a way to specify types in quotes as fully qualified names, to bypass the need for import.

[kasium]

So, how mypy resolves import is whole chapter, but no, this doesn't result in circular imports. Putting names in quotes just helps to resolve reference issues or if imports should/can not be resolved during runtime but still imports are needed.

For examples, this will fail with an unknown reference

def foo() -> Bar:
  ...

This will also fail

if TYPE_CHECKING
  import Bar

def foo() -> Bar:
  ...

This will work

if TYPE_CHECKING
  import Bar

def foo() -> 'Bar':
  ...

[yurishkuro]

Proposal - what do you suggest to solve the problem or improve the existing situation?
Add type hints 😄

I should've asked from the start - what are your exit criteria for this ticket? Was there some specific task you were trying to do that would benefit from types?

[kasium]

Valid and good question. I would say this ticket is done if all public API as discussed above is types, a py.typed is added (really minor task) and the whole stuff is released.

I used this project somewhere else and I would like to benefit from the types

[yurishkuro]

so looks like this is done?

[kasium]

Yes i think so. Can you please consider to release a new version in next time? That would be great Yuri Shkuro ***@***.***> schrieb am Fr., 10. Sept. 2021, 17:32:

…

so looks like this is done? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#319 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADZLYUWOGAV6FMMF6I5FQ6DUBIQG7ANCNFSM5B7JSSSA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[yurishkuro]

released as 4.7.0

[kasium]

Great, thanks a lot!