Admin: Enable mypy during linting #12532
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 46s ⏱️ Results for commit ac9fe3e. ♻️ This comment has been updated with latest results. |
0d6f920 to
c484266
Compare
de378b1 to
cec8172
Compare
There was a problem hiding this comment.
In my opinion, the changes are looking great! It revives and extends the mypy config from back in the days! This might as well become infectious, which would be great, but this already helps code owners to enforce typing for their codebases! 💯
I think we should really try to make this as automated as possible, few things are more frustrating than a linting check failing in CI. They should be visible asap. :)
| @@ -121,6 +121,7 @@ lint: ## Run code linter to check code style, check if formatte | |||
| ($(VENV_RUN); python -m ruff check --output-format=full . && python -m ruff format --check --diff .) | |||
| $(VENV_RUN); pre-commit run check-pinned-deps-for-needed-upgrade --files pyproject.toml # run pre-commit hook manually here to ensure that this check runs in CI as well | |||
| $(VENV_RUN); openapi-spec-validator localstack-core/localstack/openapi.yaml | |||
| $(VENV_RUN); cd localstack-core && mypy --install-types --non-interactive | |||
There was a problem hiding this comment.
question: In my opinion, everything that is checked with make lint should also be enforced by the pre-commit hook. Is there a chance to add this command to the precommit config as well? Could be tricky with the isolated execution environment though?
There was a problem hiding this comment.
It is tricky, but we can override the entry-point for the pre-commit hook, so it is possible. Updated the PR to include this, and verified locally that it works 👍
cec8172 to
3c2d248
Compare
3c2d248 to
ac9fe3e
Compare
Motivation
With #12232 merged and the
localstackpackage typed, it would be nice if the types are actually correct and verified.This PR:
Locations
I chose two locations to get this started:
localstack.packages, just because I knew that was one of the areas where typing could be improvedlocalstack.kinesis.packagesbecause it's a downstream consumer, so we also verify the usage of the package-moduleChanges
The
Packageclass is now generic, and should be typed asPackage[PackageInstaller]. This ensures thatPackage.get_installer()can return the correct subtype, likeNodePackageInstaller, and that means that consumers can also safely call any methods that are specific toNodePackageInstaller.Almost all calls to
Package.get_installed_dir()now have atype: ignoreadded to them. Theget_installed_dir-method can returnstr | None, but we assume downstream that it always returns astr.Realistically, if that method does return
None, it probably means that something went wrong during the download/installation, so we should be raising an error instead of returningNone. For now, I've decided totype: ignoreit instead of changing the behaviour (and potentially breaking other things).