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.

Sign up for GitHub

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

Jump to bottom

Add docparams lint check #145

Closed
wants to merge 10 commits into from
Closed

Add docparams lint check #145

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7fa34da
Add docparams extension, fix docs in API
c24t Sep 17, 2019
f477ff5
Fix existing lint type errors in SDK
c24t Sep 17, 2019
b3ac747
Add docs, pylint exemptions to WSGI ext
c24t Sep 17, 2019
e49a645
Restrict all lint checks to code, test dirs
c24t Sep 17, 2019
9662917
Lint setup.py files, use 2-space toxfile indent
c24t Sep 18, 2019
a51db93
Merge branch 'master' into docstring-args-lint
c24t Oct 31, 2019
bb0d2e2
Add various missing docs
c24t Nov 1, 2019
3ad9681
Merge branch 'master' into docstring-args-lint
c24t May 21, 2020
c386627
Add missing docs for docparams
c24t May 21, 2020
a167a67
Merge branch 'master' into docstring-args-lint
May 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .pylintrc
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions ext/opentelemetry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
6 changes: 6 additions & 0 deletions opentelemetry-api/src/opentelemetry/distributedcontext/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
3 changes: 3 additions & 0 deletions opentelemetry-sdk/src/opentelemetry/sdk/distributedcontext/__init__.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.

See #174 (comment)

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