Add docparams lint check #145
Conversation
c24t
requested review from
a-feld,
carlosalberto,
lzchen,
Oberon00,
reyang and
toumorokoshi
as code owners
| @@ -438,6 +438,9 @@ def use_span(self, span: "Span") -> typing.Iterator[None]: | |||
|
|
|||
| Args: | |||
| span: The span to start and make current. | |||
|
|
|||
| Yields: | |||
| `None` | |||
There was a problem hiding this comment.
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=missing-type-doc |
There was a problem hiding this comment.
This is what we get for using the same lint check in the API and SDK/etc.
There was a problem hiding this comment.
What type doc is it complaining about? Under-documented parameters. Since the Middleware is public API, we should probably really add that documentation.
tox.ini
Outdated
| @@ -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 \ | |||
There was a problem hiding this comment.
I see that was done to restrict to code and test files in e49a645 but why? Are setup.py files not code, for example?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There are probably better ways to do this, e.g. using setuptools to get a list of files for each installed package. |
|
The build is failing because the new lint check fails on changes on master. Something has gone seriously off the rails here. https://travis-ci.org/open-telemetry/opentelemetry-python/jobs/586656470 |
There was a problem hiding this comment.
Commenting to point out that I'm not approving because I dont't want these long folder lists. People should just keep their virtualenvs out of the source tree (also has the advantage that you can just run black . from a terminal without running the slower tox).
If you can get the list shorter that would also be OK.
* fix(basic-tracer): use recording/real span * fix: avoid mutate the parent span, use Object.assign()
This PR adds a lint check for missing or mismatching names in docstrings. I added this after I noticed that
opentelemetry.tracewas missing some docstring annotations. This PR doesn't quite fix the problem since we still disable themissing-docstringcheck, but it should help to keep our docstrings consistent with the code.Note that with this change we check for types (either in annotations or docstrings) in the SDK and ext packages. We may not actually want to enforce this, in which case we'll need different pylint configs for the API and SDK/etc. packages.