ci: Enable basic mypy typechecking #2385

aahung · 2020-11-18T03:02:13Z

Which issue(s) does this change fix?

Why is this change necessary?

We don't have typecheck, typecheck can make our code safer.

How does it address the issue?

Warnings and errors output by mypy without changing any code

I include a demo change how the code is safer without using Dict directly:

~~With the intro of the following dataclass, mypy can typecheck them so we won't mis-spell dict keys.~~

~~TemplateOption~~
~~RuntimeDepInfo~~
this change has been separated from this PR and moved to aahung@b5e2b1b

Some benefits we see immediately:

test_configure_samcli_logger is defined twice, and one of which cannot be triggered
test_download_source_code_binding is defined twice, and they are identical
mypy also caught a bunch of incorrect use of .format() in "tests/unit/lib/init/test_init.py"
it caught a Python 2 import
it enforces "getter" and "setter" should be next to each other, increasing readability instantly
it caught some imports that are discouraged by package developers

What side effects does this change have?

N/A

Checklist

Write design document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
make update-reproducible-reqs if dependencies were changed
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

aahung

Here are some notes about why I made these changes

aahung · 2020-11-18T03:06:55Z

samcli/lib/providers/provider.py

@@ -251,7 +251,7 @@ def binary_media_types(self):

 _CorsTuple = namedtuple("Cors", ["allow_origin", "allow_methods", "allow_headers", "max_age"])

-_CorsTuple.__new__.__defaults__ = (
+_CorsTuple.__new__.__defaults__ = (  # type: ignore


mypy won't acknowledge private attribute. So we need to disable it before we find an alternative

aahung · 2020-11-18T03:07:56Z

samcli/lib/schemas/schemas_aws_config.py

@@ -2,7 +2,7 @@

 import click

-from boto3 import Session
+from boto3.session import Session


mypy will be happier if we can import from the original class declaration file

aahung · 2020-11-18T03:08:32Z

samcli/local/common/runtime_template.py

-    for c in list(itertools.chain(*(RUNTIME_DEP_TEMPLATE_MAPPING.values())))
-    if c["dependency_manager"]
+    c.dependency_manager
+    for c in list(itertools.chain.from_iterable(RUNTIME_DEP_TEMPLATE_MAPPING.values()))


chain.from_iterable is better at perserving types than chain(*

aahung · 2020-11-18T03:09:38Z

tests/integration/local/invoke/test_integrations_cli.py

@@ -439,7 +439,7 @@ def test_invoke_returns_expected_results_from_git_function_with_parameters(self)


 class TestSamInstrinsicsAndPlugins(InvokeIntegBase):
-    template = "template-pseudo-params.yaml"
+    template = Path("template-pseudo-params.yaml")


All other subclasses of InvokeIntegBase assign template a Path value, except for this. Good catch by mypy

aahung · 2020-11-18T17:45:25Z

samcli/settings/__init__.py

+
+telemetry_endpoint_url = os.getenv(
+    "__SAM_CLI_TELEMETRY_ENDPOINT_URL", "https://aws-serverless-tools-telemetry.us-west-2.amazonaws.com/metrics"
+)


this change was addressing inconsistent types:

above: str

below: Optional[str]

aahung · 2020-11-18T17:46:01Z

samcli/local/lambdafn/env_vars.py

+    @handler.setter
+    def handler(self, value):
+        self._function["handler"] = value
+


setter / getter needs proximity

hoffa · 2020-11-18T17:31:03Z

tests/unit/lib/intrinsic_resolver/test_intrinsic_resolver.py

@@ -527,7 +530,7 @@ def test_fn_sub_second_argument_invalid_formats(self, name, intrinsic):

    @parameterized.expand(
        [
-            ("If Fn::Sub is a list, it should only have 2 arguments".format(item), item)
+            (f"If Fn::Sub is a list, it should only have 2 arguments {', '.join(item)}", item)


Why the join here but {item} a few lines up? If it doesn't matter I'd go with less code.

yeah it does not matter, I can remove the join, sounds better

tests/unit/commands/deploy/test_guided_config.py

hoffa · 2020-11-18T17:46:00Z

requirements/base.txt

@@ -15,3 +15,4 @@ requests==2.23.0
 serverlessrepo==0.1.10
 aws_lambda_builders==1.1.0
 tomlkit==0.7.0
+dataclasses~=0.8; python_version<"3.7"


Is there no other reasonable way than introducing a new dependency and using dataclasses? It seems like a reasonable change, but would be nice for the first PR to only change what's absolutely necessary.

I agree, but without getting ride of that Dict, mypy cannot infer types in quite some places..

I was aiming for NamedTuple at first, and then found they need to be mutable

dataclasses is a pretty reputable and simple portback, I think it is pretty safe to use https://github.com/ericvsmith/dataclasses

Yeah fair enough if not using it would be a significant detriment. I'm assuming without this mypy would complain a lot?

(Also if we merge this we should let the team know they're free to use dataclasses!)

You know what, I changed my mind! I think it is a great suggestion to make this PR easier to go through! I made another branch with these changes and will file a PR immediately after this one is merged 😋

They are moved here: aahung@b5e2b1b

aahung · 2020-11-18T17:46:15Z

samcli/lib/providers/provider.py

+    @codeuri.setter
+    def codeuri(self, codeuri):
+        self._codeuri = codeuri
+


setter / getter needs proximity

aahung · 2020-11-18T17:46:43Z

samcli/lib/logs/formatter.py

+
+from builtins import map


Python 2 is no longer used, mypy is not happy to see this failed import

aahung · 2020-11-18T18:25:21Z

tests/integration/buildcmd/build_integ_base.py

@@ -149,7 +149,6 @@ def _verify_invoke_built_function(self, template_path, function_logical_id, over


 class BuildIntegRubyBase(BuildIntegBase):
-    EXPECTED_FILES_GLOBAL_MANIFEST = set()


this is not being used

aahung · 2020-11-18T18:30:37Z

samcli/local/common/runtime_template.py

-SUPPORTED_DEP_MANAGERS = {
-    c["dependency_manager"]
+SUPPORTED_DEP_MANAGERS: Set[str] = {
+    c["dependency_manager"]  # type: ignore


mypy infers it as Any

aahung · 2020-11-18T18:30:49Z

samcli/local/common/runtime_template.py

-    itertools.chain(*[c["runtimes"] for c in list(itertools.chain(*(RUNTIME_DEP_TEMPLATE_MAPPING.values())))])
+RUNTIMES: Set[str] = set(
+    itertools.chain(
+        *[c["runtimes"] for c in list(itertools.chain(*(RUNTIME_DEP_TEMPLATE_MAPPING.values())))]  # type: ignore


mypy infers it as Any

aahung · 2020-11-18T18:33:53Z

samcli/lib/generated_sample_events/events.py

@@ -5,7 +5,7 @@
 import os
 import json
 import base64
-from requests.utils import quote as url_quote
+from urllib.parse import quote as url_quote


following the import in requests.utils, the quote is actually imported from urllib.parse (for Python 3)

hoffa

Great PR, thanks! 🙌 Good for ensuring long-term health.

We should also make it a soft policy to add missing types to any code touched by new PRs, so we can cover the important stuff fairly quick.

aahung · 2020-11-18T20:19:44Z

just manually merged from develop

…future (#2396) * Revert "Remove import from builtins" This reverts commit 0ecbbe4. * Revert "chore: Enable basic mypy typechecking" This reverts commit 9d29984.

aahung requested a review from hoffa November 18, 2020 03:02

aahung force-pushed the enable-mypy branch from 15435b9 to 1c2a850 Compare November 18, 2020 03:03