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

Start linting the examples folder #1701

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Start linting the examples folder #1701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1ae6a22
Start linting the examples folder
MVrachev Dec 1, 2021
d697f73
Linting in tox.ini: use variable for target dirs
MVrachev Dec 2, 2021
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
11 changes: 6 additions & 5 deletions examples/repo_example/basic_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from collections import OrderedDict
from datetime import datetime, timedelta
from pathlib import Path
from typing import Any, Dict

from securesystemslib.keys import generate_ed25519_key
from securesystemslib.signer import SSlibSigner
Expand All @@ -48,7 +49,7 @@
from tuf.api.serialization.json import JSONSerializer


def _in(days):
def _in(days: float) -> datetime:
"""Adds 'days' to now and returns datetime object w/o microseconds."""
return datetime.utcnow().replace(microsecond=0) + timedelta(days=days)

Expand Down Expand Up @@ -89,8 +90,8 @@ def _in(days):

# Define containers for role objects and cryptographic keys created below. This
# allows us to sign and write metadata in a batch more easily.
roles = {}
keys = {}
roles: Dict[str, Metadata] = {}
keys: Dict[str, Dict[str, Any]] = {}
Copy link
Member

Choose a reason for hiding this comment

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

Quick question, what is the criterion for a variable name to get type annotated?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@MVrachev MVrachev Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.



# Targets (integrity)
Expand All @@ -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))
Copy link
Member

Choose a reason for hiding this comment

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

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.)

Copy link
Collaborator Author

@MVrachev MVrachev Dec 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, mypy noticed that.
Yet another argument as to why we do need linting quickly as possible on new code.

roles["targets"].signed.targets[target_path] = target_file_info

# Snapshot (consistency)
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

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

👀 Whoopsi, thanks for catching this! Did mypy detect it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it did.



# Snapshot + Timestamp + Sign + Persist
Expand Down
5 changes: 0 additions & 5 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,6 @@ strict_equality = "True"
disallow_untyped_defs = "True"
disallow_untyped_calls = "True"
show_error_codes = "True"
files = [
"tuf/api/",
"tuf/ngclient",
"tuf/exceptions.py"
]

[[tool.mypy.overrides]]
module = [
Expand Down
9 changes: 5 additions & 4 deletions tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,18 @@ commands =

[testenv:lint]
changedir = {toxinidir}
lint_dirs = tuf/api tuf/ngclient examples
commands =
# Use different configs for new (tuf/api/*) and legacy code
black --check --diff tuf/api tuf/ngclient
isort --check --diff tuf/api tuf/ngclient
pylint -j 0 tuf/api tuf/ngclient --rcfile=pyproject.toml
black --check --diff {[testenv:lint]lint_dirs}
isort --check --diff {[testenv:lint]lint_dirs}
pylint -j 0 --rcfile=pyproject.toml {[testenv:lint]lint_dirs}

# NOTE: Contrary to what the pylint docs suggest, ignoring full paths does
# work, unfortunately each subdirectory has to be ignored explicitly.
pylint -j 0 tuf --ignore=tuf/api,tuf/api/serialization,tuf/ngclient,tuf/ngclient/_internal

mypy
mypy {[testenv:lint]lint_dirs} tuf/exceptions.py

bandit -r tuf

Expand Down