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 mypy doc #1611

Merged
merged 4 commits into from
Aug 24, 2022
Merged

Add mypy doc #1611

merged 4 commits into from
Aug 24, 2022

Conversation

cartermp
Copy link
Contributor

@cartermp cartermp commented Aug 11, 2022

This came up before, but there was some recent conversation here that made me feel like we could do a quick doc explaining how to configure a mypy project: open-telemetry/opentelemetry-python#2591 (comment)

There's mention of potentially needing this in a project configuration file:

[mypy-opentelemetry.sdk.*]
implicit_reexport = True

I do not know if this is also necessary, but it should be easy to add if it is.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

See inline copyedit comments. Thanks!

content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
content/en/docs/instrumentation/python/mypy.md Outdated Show resolved Hide resolved
@chalin
Copy link
Contributor

chalin commented Aug 11, 2022

Broader comment: As it stands the added page is very specific. Might it make sense to add the content to another page (such as the Python landing page), or to generalize the page title? Just a thought, I'm ok if this lands with the current page addition if others are ok with that too.

@cartermp
Copy link
Contributor Author

I'd like to get the @open-telemetry/python-approvers take on this being a specific page. One way to look at mypy is that it's analogous to TypeScript, which is to say it's a dialect of the same core/family of language. And as it turns out, there are some unique things to keep in mind when using it. We have the same thing to consider for the JS docs long-term as well.

@sk-
Copy link

sk- commented Aug 12, 2022

There's mention of potentially needing this in a project configuration file:

[mypy-opentelemetry.sdk.*]
implicit_reexport = True

I do not know if this is also necessary, but it should be easy to add if it is.

This is absolutely necessary if you are using the strict option, because it enables the flag --no-implicit-reexport.

This seems to be required only for the sdk because it programmatically constructs the __all__ variable, as opposed to the api where the __all__ variable is explicitly defined.

Without this option one gets errors like the following:

app/otel.py:7: error: Module "opentelemetry.sdk.metrics" does not explicitly export attribute "MeterProvider"; implicit reexport disabled  [attr-defined]
app/otel.py:8: error: Module "opentelemetry.sdk.metrics.export" does not explicitly export attribute "PeriodicExportingMetricReader"; implicit reexport disabled  [attr-defined]
app/otel.py:13: error: Module "opentelemetry.sdk.metrics.view" does not explicitly export attribute "ExplicitBucketHistogramAggregation"; implicit reexport disabled  [attr-defined]

@cartermp
Copy link
Contributor Author

@chalin and @svrnm your feedback should be addressed.

@open-telemetry/python-approvers any feedback on the doc?

Copy link
Member

@svrnm svrnm left a comment

Choose a reason for hiding this comment

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

lgtm

@chalin
Copy link
Contributor

chalin commented Aug 22, 2022

@open-telemetry/python-approvers - any feedback on this?

@ocelotl
Copy link
Contributor

ocelotl commented Aug 23, 2022

@open-telemetry/python-approvers - any feedback on this?

Looks good to me initially. Just a few things:

Is the underlying intention of this documentation to explain how to use mypy in a "standalone" way? What I mean is that we use mypy as part of our regular testing process, we do this by runnning tox -e mypy. I can imagine someone wants to run mypy by themselves on OTel Python by I wonder what is the context behind doing so.

If the intent of running mypy is to make sure everything is ok in order to submit a PR to collaborate with the project (I imagine that is not the case), then this information should be here.

Regarding this comment, we actually should update the API to also dynamically generate the contents of __all__ to be consistent with the SDK.

@cartermp
Copy link
Contributor Author

@ocelotl The intention here is around end-user use of the SDK when instrumenting. I also had a coworker try out otel-python for themselves in a flask app and they ran into the same issue, needing to add this to their configuration to unblock themselves.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks all!

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.

None yet

5 participants