Skip to content

Add type check to travis #31

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

Merged
merged 6 commits into from
Jun 27, 2019
Merged

Add type check to travis #31

merged 6 commits into from
Jun 27, 2019

Conversation

c24t
Copy link
Member

@c24t c24t commented Jun 26, 2019

Piling onto #30, this PR adds type checks via mypy to travis for the API package.

These are pretty strict defaults; I followed @Oberon00's approach in #24 and enabled every available option that wasn't already incompatible with our code. I had to omit --disallow-any-expr because of contextmanager, but there may still be a way around this.

See https://mypy.readthedocs.io/en/latest/config_file.html for details on these options.

@c24t c24t added the meta Related to repo itself, process, community, ... label Jun 26, 2019
@Oberon00
Copy link
Member

A potential way around this that I can see is using typing.ContextManager[Span] instead of @contextmanager and Iterator[Span]. typing.ContextManager is only available since Python 3.5.4 though and I think we would need more boilerplate code. The 3.5.4 part could probably be worked around with some if MYPY which we should consider anyway, since typing is new in 3.5. Or we use https://pypi.org/project/typing/ and typing.IS_TYPE_CHECKING (3.5.2)

Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly straightforward. The only thing that irks me is the #type: ignore in the __init__.pys. I wonder why mypy has problems with __path__. A mypy bug?

@c24t c24t force-pushed the add-mypy-check branch from 26be5f8 to dfe87df Compare June 26, 2019 21:10
Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@c24t
Copy link
Member Author

c24t commented Jun 26, 2019

typing.ContextManager is only available since Python 3.5.4 though and I think we would need more boilerplate code

I don't know that it's worth it to add the extra boilerplate, and it looks like mypy correctly checks calls to e.g. start_span even though the context manager itself isn't typed. I'll leave it as-is and we can revisit if this causes problems down the road.

The 3.5.4 part could probably be worked around with some if MYPY which we should consider anyway, since typing is new in 3.5

It sounds like we've got two options: either install the typing backport if we're running on 3.4 or eat the error while importing typing and change references to the module in type annotations to strings. So from:

    def start_span(self, name: str, parent: 'Span') -> typing.Iterator[Span]:

to:

    def start_span(self, name: str, parent: 'Span') -> 'typing.Iterator[Span]':

The second option seems like overkill and doesn't let us use e.g. typing.ContextManager, but requiring a third party library for optional type checks doesn't sound any better.

@c24t
Copy link
Member Author

c24t commented Jun 26, 2019

The only thing that irks me is the #type: ignore in the __init__.pys. I wonder why mypy has problems with __path__. A mypy bug?

Luckily this is fixed -- for now -- because of the package move. It does look like this is an open bug in mypy: python/mypy#1422.

@c24t
Copy link
Member Author

c24t commented Jun 26, 2019

I added typing as a requirement for pre-3.5 python versions in 15b30bc.

@c24t c24t merged commit 0bf0fad into open-telemetry:master Jun 27, 2019
@c24t c24t deleted the add-mypy-check branch June 27, 2019 16:38
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* Add initial span interface

* Add object type attribute

* return this and add optional endTime

* Use any and add TODO

* Update TraceState with examples and add TODO

* specify traceId and spanId format

* use unknown instead of any

* Add TraceOptions and TraceState empty class

* Add Link and Attributes interfaces

* update index.ts

* Change Link:attributes as optional property

* Fix review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Related to repo itself, process, community, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants