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 docparams lint check #145

Closed
wants to merge 10 commits into from
5 changes: 3 additions & 2 deletions .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ limit-inference-results=100

# List of plugins (as comma separated values of python modules names) to load,
# usually to register additional checkers.
load-plugins=
load-plugins=pylint.extensions.docparams

# Pickle collected data for later comparisons.
persistent=yes
Expand Down Expand Up @@ -68,7 +68,8 @@ disable=missing-docstring,
ungrouped-imports, # Leave this up to isort
wrong-import-order, # Leave this up to isort
bad-continuation, # Leave this up to black
line-too-long # Leave this up to black
line-too-long, # Leave this up to black
Oberon00 marked this conversation as resolved.
Show resolved Hide resolved
missing-yield-type-doc # Use return type annotation instead

# Enable the message, report, category or checker with the given id(s). You can
# either give multiple identifier separated by comma (,) or put this option
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@ class OpenTelemetryMiddleware:

Args:
wsgi: The WSGI application callable.
propagators: TODO
"""

# pylint: disable=missing-type-doc
Copy link
Member Author

Choose a reason for hiding this comment

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

This is what we get for using the same lint check in the API and SDK/etc.

Copy link
Member

Choose a reason for hiding this comment

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

What type doc is it complaining about? Under-documented parameters. Since the Middleware is public API, we should probably really add that documentation.

def __init__(self, wsgi, propagators=None):
self.wsgi = wsgi

Expand Down Expand Up @@ -77,12 +79,16 @@ def _start_response(status, response_headers, *args, **kwargs):

return _start_response

# pylint: disable=missing-type-doc
def __call__(self, environ, start_response):
"""The WSGI application

Args:
environ: A WSGI environment.
start_response: The WSGI start_response callable.

Yields:
Zero or more strings that comprise the WSGI response.
"""

tracer = trace.tracer()
Expand Down
11 changes: 5 additions & 6 deletions opentelemetry-api/src/opentelemetry/context/base_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,12 @@ def register_slot(
) -> "BaseRuntimeContext.Slot":
"""Register a context slot with an optional default value.

:type name: str
:param name: The name of the context slot.
Args:
name: The name of the context slot.
default: The default value of the slot, can be a value or lambda.

:type default: object
:param name: The default value of the slot, can be a value or lambda.

:returns: The registered slot.
Returns:
The registered slot.
"""
with cls._lock:
if name not in cls._slots:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ def get_entry_value(self, key: EntryKey) -> typing.Optional[EntryValue]:

Args:
key: the key with which to perform a lookup

Returns:
The value of the entry associated with `key`
"""
if key in self._container:
return self._container[key].value
Expand Down Expand Up @@ -120,6 +123,9 @@ def use_context(

Args:
context: A DistributedContext instance to make current.

Yields:
The current DistributedContext.
"""
# pylint: disable=no-self-use
yield context
10 changes: 8 additions & 2 deletions opentelemetry-api/src/opentelemetry/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,8 @@ class SpanContext:
Args:
trace_id: The ID of the trace that this span belongs to.
span_id: This span's ID.
options: Trace options to propagate.
state: Tracing-system-specific info to propagate.
trace_options: Trace options to propagate.
trace_state: Tracing-system-specific info to propagate.
"""

def __init__(
Expand Down Expand Up @@ -438,6 +438,9 @@ def use_span(self, span: "Span") -> typing.Iterator[None]:

Args:
span: The span to start and make current.

Yields:
`None`
Copy link
Member Author

Choose a reason for hiding this comment

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

Backticks here because this is interpreted as a text description of the thing that we yield. None: some description would force "None" to render as a type.

Ideally we'd pick up the yielded type from the return type annotation, but sphinx_autodoc_typehints isn't smart enough to unpack -> typing.Iterator[None] above.

"""
# pylint: disable=unused-argument,no-self-use
yield
Expand Down Expand Up @@ -480,6 +483,9 @@ def set_preferred_tracer_implementation(

Args:
factory: Callback that should create a new :class:`Tracer` instance.

Raises:
RuntimeError: On repeated attempts to load the tracer.
"""
global _TRACER_FACTORY # pylint:disable=global-statement

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ def use_context(

Args:
context: A DistributedContext instance to make current.

Yields:
The current DistributedContext.
"""
snapshot = self._current_context.get()
self._current_context.set(context)
Expand Down
10 changes: 5 additions & 5 deletions opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -235,9 +235,9 @@ def __init__(
name: str,
context: "trace_api.SpanContext",
parent: trace_api.ParentSpan = None,
sampler=None, # TODO
trace_config=None, # TODO
resource=None, # TODO
sampler: object = None, # TODO
trace_config: object = None, # TODO
resource: object = None, # TODO
attributes: types.Attributes = None, # TODO
events: typing.Sequence[trace_api.Event] = None, # TODO
links: typing.Sequence[trace_api.Link] = None, # TODO
Expand Down Expand Up @@ -363,7 +363,7 @@ def update_name(self, name: str) -> None:
self.name = name


def generate_span_id():
def generate_span_id() -> int:
"""Get a new random span ID.

Returns:
Expand All @@ -372,7 +372,7 @@ def generate_span_id():
return random.getrandbits(64)


def generate_trace_id():
def generate_trace_id() -> int:
"""Get a new random trace ID.

Returns:
Expand Down
42 changes: 37 additions & 5 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -73,18 +73,50 @@ commands_pre =
commands =
; Prefer putting everything in one pylint command to profit from duplication
; warnings.
black --check --diff .
pylint opentelemetry-api/src/opentelemetry \
black --check --diff \
Copy link
Member

Choose a reason for hiding this comment

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

I see that was done to restrict to code and test files in e49a645 but why? Are setup.py files not code, for example?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it'd be fine to include setup.py here, this was just copied from the pylint args below.

e49a645 was because tox was failing on my machine, linting a bunch of files in virtualenvs and etc. This seemed like a straightforward improvement to me, even if it means a bit more work keeping the toxfile up to date with the package structure.

Copy link
Member

Choose a reason for hiding this comment

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

I had that problem with virtualenvs too but decided that keeping virtualenvs out of the project dir is easier than maintaining theses ugly lists (separation of source and build tree is a nice thing in general).

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I also vote for not adding it this way. This repo may not add many more directories, but it's quite tricky to add the correct ones and increases the size and error-prone boilerplate considerably.

Maybe we should discuss how we expect to do venvs though? I like using vscode and it's autocompletion doesn't find modules at all with the current structure.

opentelemetry-api/src/opentelemetry \
opentelemetry-api/tests/ \
opentelemetry-sdk/src/opentelemetry \
opentelemetry-sdk/tests/ \
ext/opentelemetry-ext-http-requests/src/ \
ext/opentelemetry-ext-http-requests/src/opentelemetry/ \
ext/opentelemetry-ext-http-requests/tests/ \
ext/opentelemetry-ext-wsgi/src/opentelemetry/ \
ext/opentelemetry-ext-wsgi/tests/ \
opentelemetry-example-app/src/opentelemetry_example_app/ \
opentelemetry-example-app/tests/
flake8 .
isort --check-only --diff --recursive .
pylint \
opentelemetry-api/src/opentelemetry \
opentelemetry-api/tests/ \
opentelemetry-sdk/src/opentelemetry \
opentelemetry-sdk/tests/ \
ext/opentelemetry-ext-http-requests/src/opentelemetry/ \
ext/opentelemetry-ext-http-requests/tests/ \
ext/opentelemetry-ext-wsgi/src/opentelemetry/ \
ext/opentelemetry-ext-wsgi/tests/ \
opentelemetry-example-app/src/opentelemetry_example_app/ \
opentelemetry-example-app/tests/
flake8 \
opentelemetry-api/src/opentelemetry \
opentelemetry-api/tests/ \
opentelemetry-sdk/src/opentelemetry \
opentelemetry-sdk/tests/ \
ext/opentelemetry-ext-http-requests/src/opentelemetry/ \
ext/opentelemetry-ext-http-requests/tests/ \
ext/opentelemetry-ext-wsgi/src/opentelemetry/ \
ext/opentelemetry-ext-wsgi/tests/ \
opentelemetry-example-app/src/opentelemetry_example_app/ \
opentelemetry-example-app/tests/
isort --check-only --diff --recursive \
opentelemetry-api/src/opentelemetry \
opentelemetry-api/tests/ \
opentelemetry-sdk/src/opentelemetry \
opentelemetry-sdk/tests/ \
ext/opentelemetry-ext-http-requests/src/opentelemetry/ \
ext/opentelemetry-ext-http-requests/tests/ \
ext/opentelemetry-ext-wsgi/src/opentelemetry/ \
ext/opentelemetry-ext-wsgi/tests/ \
opentelemetry-example-app/src/opentelemetry_example_app/ \
opentelemetry-example-app/tests/

[testenv:docs]
deps =
Expand Down