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

More lint #43

Merged
merged 7 commits into from
Jul 15, 2019
Merged

More lint #43

merged 7 commits into from
Jul 15, 2019

Conversation

Oberon00
Copy link
Member

@Oberon00 Oberon00 commented Jul 8, 2019

Building on #42 (which in turn builds on #29, sorry): lint code in setup.py, docs/conf.py.

This does not add linting to the stub opentelemetry-sdk package.

Why would we ignore our line length limits in setup.py?

Also use more mypy checks for test code (without check_untyped_defs
it's really useless).
docs/conf.py Outdated
@@ -18,7 +18,7 @@
# -- Project information -----------------------------------------------------

project = 'OpenTelemetry'
copyright = '2019, OpenTelemetry Authors'
copyright = '2019, OpenTelemetry Authors' #pylint:disable=redefined-builtin
Copy link
Member

Choose a reason for hiding this comment

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

We should probably just not lint docs/.

@c24t
Copy link
Member

c24t commented Jul 10, 2019

Looks good, but why not disable type checking for unit tests altogether?

@Oberon00
Copy link
Member Author

We could disable type checking but I think it might be good to enable it, also to find issues with type hints in the API (for example, it might be possible to define a type hint that works for the function definition but is actually too strict for typical usages)

@c24t
Copy link
Member

c24t commented Jul 11, 2019

To find issues with the types in the API I think it would be great to have run time type checking during tests, that way we could write tests without type annotations but still benefit from the checks. But I don't see any non-hacky way to do this.

@Oberon00
Copy link
Member Author

The static type (the one that the type checker infers) could still be different from the runtime type. The typical usage for type hints that I would expect is that users run mypy over their code and are happy that they don't have to put opentelemetry-python on the ignore list.

Also, I hope that even without explicit type hints mypy can infer many types (e.g. it should know that a local variable x = trace.tracer() has type Tracer, thanks to the return type annotation of tracer()).

@c24t
Copy link
Member

c24t commented Jul 15, 2019

@reyang, @carlosalberto let me know if this LGTY and we can merge it.

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 c24t merged commit 677e848 into open-telemetry:master Jul 15, 2019
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.

3 participants