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

Update docs for context lib #73

Merged
merged 5 commits into from
Aug 14, 2019
Merged

Update docs for context lib #73

merged 5 commits into from
Aug 14, 2019

Conversation

reyang
Copy link
Member

@reyang reyang commented Aug 7, 2019

This should address most of the context related issues discovered by @c24t in #70.

Part of the effort for #22.

@Oberon00
Copy link
Member

Oberon00 commented Aug 8, 2019

Does this PR depend on or replace #70?

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.

Apart from the two typos that travis/Sphinx complains about (looks like BaseRuntimeContext is missing), LGTM.

@reyang
Copy link
Member Author

reyang commented Aug 8, 2019

Does this PR depend on or replace #70?

Neither, it is trying to make it easier for #70. We don't expect @c24t to fix all the doc issues for context and metrics.

@reyang
Copy link
Member Author

reyang commented Aug 8, 2019

The remaining failures seem like a Sphinx bug:

/home/travis/build/open-telemetry/opentelemetry-python/opentelemetry-api/src/opentelemetry/context/async_context.py:docstring
of opentelemetry.context.async_context.AsyncRuntimeContext.Slot:1: WARNING:
py:class reference target not found: opentelemetry.context.base_context.Slot
/home/travis/build/open-telemetry/opentelemetry-python/opentelemetry-api/src/opentelemetry/context/thread_local_context.py:docstring
of opentelemetry.context.thread_local_context.ThreadLocalRuntimeContext.Slot:1: WARNING:
py:class reference target not found: opentelemetry.context.base_context.Slot

It seems Sphinx got confused while trying to find the base class "class reference target not found: opentelemetry.context.base_context.Slot", the path is wrong.

@c24t any idea how to workaround this? I'll search and see if there are some solution.
BTW, I think we probably don't need to document the actual context implementation, the module level doc should provide sufficient information.

@c24t
Copy link
Member

c24t commented Aug 9, 2019

That looks like a sphinx bug to me, you may be able to track down the problem in https://github.com/agronholm/sphinx-autodoc-typehints/blob/master/sphinx_autodoc_typehints.py.

Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

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

The changes LGTM, but hopefully we can find a way to fix the errors in sphinx before merging this to avoid having a broken build on master. If there's no way around it we can also disable the docs build check for now.

@reyang
Copy link
Member Author

reyang commented Aug 14, 2019

I've temporarily removed the thread_local_context and async_context from the doc tree to unblock the PR and CI.

@reyang reyang merged commit 2edc07f into master Aug 14, 2019
@reyang reyang deleted the context-docs branch August 14, 2019 00:32
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
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