Even more mypy type checking #1238

doshitan · 2020-03-02T04:56:18Z

It started out a little more disciplined then became a big ball of changes, apologies.

Also some of this might be overkill.

Included the stricter mypy config that this passes for reference, but it's maybe we don't want to require it yet.

Running mypy (with stricter config):

The mypy report part errors out early, but what it does get to:

+--------------------------+-------------------+----------+
| Module                   | Imprecision       | Lines    |
+--------------------------+-------------------+----------+
| nixops                   |   0.00% imprecise |    0 LOC |
| nixops.ansi              |   0.00% imprecise |   18 LOC |
| nixops.diff              |  15.17% imprecise |  211 LOC |
| nixops.known_hosts       |  11.43% imprecise |   70 LOC |
| nixops.logger            |   0.00% imprecise |  158 LOC |
| nixops.nix_expr          |  27.53% imprecise |  385 LOC |
| nixops.parallel          |   9.48% imprecise |  116 LOC |
| nixops.plugins           |  58.33% imprecise |   12 LOC |
| nixops.plugins.hookspecs |  15.38% imprecise |   26 LOC |
| nixops.resources         |  15.31% imprecise |  320 LOC |
| nixops.script_defs       |  28.99% imprecise | 1128 LOC |
| nixops.ssh_util          |   5.62% imprecise |  409 LOC |
| nixops.state             |  18.07% imprecise |   83 LOC |
| nixops.util              |  10.83% imprecise |  600 LOC |
+--------------------------+-------------------+----------+
| Total                    |  18.30% imprecise | 3536 LOC |
+--------------------------+-------------------+----------+

Tests:

@grahamc saw your tweet about working on this and having some experience hitting python over the head with mypy, figured I'd make a pass (^_^)

grahamc · 2020-03-02T13:13:08Z

This is amazing! There are some module import errors which I can help track down and fix. Thank you!

grahamc · 2020-03-02T14:03:36Z

nixops/parallel.py

+) -> FinalResult[Result]:
    task_queue: queue.Queue[Task] = queue.Queue()
-    result_queue: queue.Queue[WorkerResult] = queue.Queue()
+    result_queue: queue.Queue[WorkerResult[Result]] = queue.Queue()


I'm a bit surprised -- is FinalResult / WorkerResult a generic type?

WorkerResult is defined a little further up, it's basically an Either from Haskell, or std::variant from C++.

nixops/logger.py

nixops/nix_expr.py

TheWizardTower · 2020-03-02T15:21:34Z

nixops/nix_expr.py

+    @overload
+    def _enc_str(node: str, for_attribute: Literal[True]) -> str:
+        ...
+
+    @overload
+    def _enc_str(
+        node: str, for_attribute: Literal[False]
+    ) -> Union[RawValue, Container]:
+        ...
+
+    @overload
+    def _enc_str(node: str) -> Union[RawValue, Container]:
+        ...
+
+    def _enc_str(
+        node: str, for_attribute: bool = False
+    ) -> Union[str, RawValue, Container]:


I'm curious to try this overall PR without the Literal import, given that it's only used in two places. If that works, we can merge the rest and make this it's own PR and figure out if/how python does dependency management.

Yea, I'd kinda prefer separate functions with it clear in the name what they do/return, rather than relying on a special parameter, but the pattern was present in a variety of places and I don't know if there are calls to this stuff (particularly the logged_exec calls/wrappers) outside of this code base, so was hesitant to change function names.

If we can refactor and do away with all the Literal junk without also adding a bunch of casts, I think that'd be a win.

@overload

An alternative implementation of NixOS#1238 from GitHub user doshitan which used Literal[...] as a sort of dependent type: + @overload + def run_command( + self, + command: Command, + flags: List[str] = ..., + timeout: Optional[int] = ..., + logged: bool = ..., + allow_ssh_args: bool = ..., + user: Optional[str] = ..., + *, + capture_stdout: Literal[False], + **kwargs: Any + ) -> int: + ... + + @overload + def run_command( + self, + command: Command, + flags: List[str] = ..., + timeout: Optional[int] = ..., + logged: bool = ..., + allow_ssh_args: bool = ..., + user: Optional[str] = ..., + *, + capture_stdout: Literal[True], + **kwargs: Any + ) -> str: + ... + + @overload + def run_command( + self, + command: Command, + flags: List[str] = [], + timeout: Optional[int] = None, + logged: bool = True, + allow_ssh_args: bool = False, + user: Optional[str] = None, + **kwargs: Any + ) -> Union[str, int]: + ... however, I think this option is much simpler and improves the API. Possibly in the future we will add this more complex type.

@overload

An alternative implementation of #1238 from GitHub user doshitan which used Literal[...] as a sort of dependent type: + @overload + def run_command( + self, + command: Command, + flags: List[str] = ..., + timeout: Optional[int] = ..., + logged: bool = ..., + allow_ssh_args: bool = ..., + user: Optional[str] = ..., + *, + capture_stdout: Literal[False], + **kwargs: Any + ) -> int: + ... + + @overload + def run_command( + self, + command: Command, + flags: List[str] = ..., + timeout: Optional[int] = ..., + logged: bool = ..., + allow_ssh_args: bool = ..., + user: Optional[str] = ..., + *, + capture_stdout: Literal[True], + **kwargs: Any + ) -> str: + ... + + @overload + def run_command( + self, + command: Command, + flags: List[str] = [], + timeout: Optional[int] = None, + logged: bool = True, + allow_ssh_args: bool = False, + user: Optional[str] = None, + **kwargs: Any + ) -> Union[str, int]: + ... however, I think this option is much simpler and improves the API. Possibly in the future we will add this more complex type.

grahamc

There is definitely good stuff in here, though it became a bit tricky to merge the later commits, for sure. I picked out some of the easy-to-check, easy to break-out changes and merged those. I'm hoping to merge more from this PR, too.

grahamc · 2020-03-03T00:22:32Z

nixops/nix_expr.py

        return "  " * level + self.value

-    def __repr__(self):
+    def __repr__(self) -> str:


I'm a bit unsure about this -- we say we're returning a str, but above we accept an Any in the constructor. What do you think?

My understanding of Python is that __repr__ should always return a string (and typeshed seems to agree). But yea, strange that it just returns the raw value and I don't know if the behavior is relied on somewhere :/

It'll take more types to handle things like `{("a", "b"): 1, "a": 2}` properly, for now, loosen types/go back to more like it was before.

For Literal support in Python 3.7 (though we could also just drop usage of Literal)

…start-with-types * origin/start-with-types: ssh_util: typed run_command wrapper ssh_util.py logger.py nixops: make the op required

doshitan · 2020-03-03T03:02:51Z

Welp, down to just one coverage test error (due to more import cycle shenanigans).

I don't know why this was done back in 2015 or how it's used, but it fixes the coverage tests locally.

@doshitan

Partially apply @doshitan's #1238 ... part 2

grahamc · 2020-04-15T15:57:12Z

I'm remiss to close this, but it is a bit out of date from master and the remaining commits are tricky to merge. Maybe the work here could be broken up in to a few smaller and related chunks?

doshitan · 2020-04-15T22:12:50Z

Yea, I've not had a chance to get back to this lately, if you've already lifted what useful bits you could then feel free to close! I may have time this weekend to rebase/pick out any standalone chunks, but can't promise anything.

grahamc · 2020-04-20T18:23:58Z

If you rebase, go ahead and reopen? Thank you!

doshitan added 6 commits March 1, 2020 23:14

nix_expr.py

d39cdb3

parallel.py

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

74767c1

logger.py

7897440

ssh_util.py

d1cda7e

Moar types

91c11a2

Stricter mypy config

9c5a852

grahamc reviewed Mar 2, 2020

View reviewed changes

nixops/logger.py Outdated Show resolved Hide resolved

TheWizardTower reviewed Mar 2, 2020

View reviewed changes

nixops/nix_expr.py Show resolved Hide resolved

TheWizardTower reviewed Mar 2, 2020

View reviewed changes

grahamc mentioned this pull request Mar 2, 2020

Partially apply doshitan's #1238 part 1 #1239

Merged

grahamc reviewed Mar 3, 2020

View reviewed changes

doshitan added 6 commits March 2, 2020 20:53

Fix import issues

159eb2c

Fix MultiLineRawValue

a7ea182

Fix nixmerge

258cb72

It'll take more types to handle things like `{("a", "b"): 1, "a": 2}` properly, for now, loosen types/go back to more like it was before.

Add typing_extensions package to deps

3327dee

For Literal support in Python 3.7 (though we could also just drop usage of Literal)

Merge remote-tracking branch 'origin/start-with-types' into doshitan/…

2476e0d

…start-with-types * origin/start-with-types: ssh_util: typed run_command wrapper ssh_util.py logger.py nixops: make the op required

Fix more import stuff

eb4bd57

Try disabling dynamic resource load in deployment.py

a0c9603

I don't know why this was done back in 2015 or how it's used, but it fixes the coverage tests locally.

grahamc added a commit that referenced this pull request Mar 9, 2020

Merge pull request #1241 from NixOS/partially-apply-doshitan-1238-part-2

f548942

Partially apply @doshitan's #1238 ... part 2

grahamc closed this Apr 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Even more mypy type checking #1238

Even more mypy type checking #1238

doshitan commented Mar 2, 2020 •

edited

Loading

grahamc commented Mar 2, 2020

grahamc Mar 2, 2020

TheWizardTower Mar 2, 2020

TheWizardTower Mar 2, 2020

doshitan Mar 3, 2020

grahamc left a comment

grahamc Mar 3, 2020

doshitan Mar 3, 2020

doshitan commented Mar 3, 2020

grahamc commented Apr 15, 2020

doshitan commented Apr 15, 2020

grahamc commented Apr 20, 2020

Even more mypy type checking #1238

Even more mypy type checking #1238

Conversation

doshitan commented Mar 2, 2020 • edited Loading

grahamc commented Mar 2, 2020

grahamc Mar 2, 2020

Choose a reason for hiding this comment

TheWizardTower Mar 2, 2020

Choose a reason for hiding this comment

TheWizardTower Mar 2, 2020

Choose a reason for hiding this comment

doshitan Mar 3, 2020

Choose a reason for hiding this comment

grahamc left a comment

Choose a reason for hiding this comment

grahamc Mar 3, 2020

Choose a reason for hiding this comment

doshitan Mar 3, 2020

Choose a reason for hiding this comment

doshitan commented Mar 3, 2020

grahamc commented Apr 15, 2020

doshitan commented Apr 15, 2020

grahamc commented Apr 20, 2020

doshitan commented Mar 2, 2020 •

edited

Loading