Start linting the examples folder #1701
Conversation
Pull Request Test Coverage Report for Build 1530507142
💛 - Coveralls |
pyproject.toml
Outdated
| files = [ | ||
| "tuf/api/", | ||
| "tuf/ngclient", | ||
| "tuf/exceptions.py" | ||
| "tuf/exceptions.py", | ||
| "examples/", | ||
| ] |
There was a problem hiding this comment.
left a comment about this in the other bug: #1699 (comment) (we could remove these and just define the "linter dirs" in one place in tox.ini to make maintenance easier)
| @@ -332,7 +333,7 @@ def _in(days): | |||
| del roles["targets"].signed.targets[target_path] | |||
|
|
|||
| # Increase expiry (delegators should be less volatile) | |||
| roles["targets"].expires = _in(365) | |||
| roles["targets"].signed.expires = _in(365) | |||
There was a problem hiding this comment.
👀 Whoopsi, thanks for catching this! Did mypy detect it?
| @@ -117,7 +118,7 @@ def _in(days): | |||
| local_path = Path(__file__).resolve() | |||
| target_path = f"{local_path.parts[-2]}/{local_path.parts[-1]}" | |||
|
|
|||
| target_file_info = TargetFile.from_file(target_path, local_path) | |||
| target_file_info = TargetFile.from_file(target_path, str(local_path)) | |||
There was a problem hiding this comment.
Was this also flagged by mypy?
(Also it is odd, because I remember already casting this to str at some point because TargetFile.from_file raised an error with local_path being a Path object. But I can't find it in #1685. I wonder were it went... Anyways, good that you/mypy fixed it.)
There was a problem hiding this comment.
Yes, mypy noticed that.
Yet another argument as to why we do need linting quickly as possible on new code.
| roles = {} | ||
| keys = {} | ||
| roles: Dict[str, Metadata] = {} | ||
| keys: Dict[str, Dict[str, Any]] = {} |
There was a problem hiding this comment.
Quick question, what is the criterion for a variable name to get type annotated?
There was a problem hiding this comment.
it's needed if mypy can't figure it out from the context (and mypy is not very advanced in this). So in this case roles = { "root": root_md } would have been fine but an empty container needs typing
There was a problem hiding this comment.
I think it does need annotation when you use containers and personally, I think it's better to define annotations in these cases. It makes it so much easier to understand how the container should be used or is used later.
The examples folder currently contains a repository example and it's good if we start linting its content and as a result add type annotations. Signed-off-by: Martin Vrachev <[email protected]>
Instead of providing a target directory for linting by each of the tools use one variable which will be the source of truth about which directories do we lint. Signed-off-by: Martin Vrachev <[email protected]>
MVrachev
force-pushed
the
lint-examples
branch
from
481f45c to
d697f73
Compare
|
I rebased on top of develop and created a commit adding a This pr is ready for review. |
Pull Request Test Coverage Report for Build 1530507142Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Fixes #1697
Description of the changes being introduced by the pull request:
The examples folder currently contains a repository example and it's
good if we start linting its content and as a result add type
annotations.
I run all of the linters on the example folder instead of only using
mypyas I thought that will be better for the code included there.
Signed-off-by: Martin Vrachev [email protected]
Please verify and check that the pull request fulfills the following
requirements: