2018, Jun 11 - Dimitri Merejkowsky License: CC By 4.0
This is a follow up to the I don't need types[1] article.
I left a teaser explaining I'll be giving concrete examples using a Python project, so here goes.
As you may have guessed, I'm going to talk about type annotations and the mypy[2] static type checker.
Here's a quick overview of how mypy works, in case you are not already familiar with it.
Let's use a rather contrived example:
def is_odd(num):
return num % 2 == 0
if is_odd("this sentence contains %s"):
print("ok")
We have a function `is_odd`, which obviously only works with numbers, and we call it with a string.
But since the string contains `%s`, Python will happily assume we are trying to format it, and the bug will go unnoticed. `is_odd` will simply return False, because we are *also* allowed to compare strings and numbers with `==` in Python.
Without type annotations, mypy detects nothing:
$ mypy foo.py <nothing>
But if we add type annotations and re-run mypy, we do get an error message:
def is_odd(num: int) -> bool:
return num % 2 == 0
if is_odd("this sentence contains %s"):
print("ok")
$ mypy foo.py error: Argument 1 to "is_odd" has incompatible type "str"; expected "int"
Thus, you can use mypy in a loop:
This is called "gradual typing".
The goal of the experiment is to check if going through the trouble of adding type annotations is worth it.
Let me define what I mean by "worth it".
There are many things said about types annotations like:
Those may be true, but they are hard to measure.
So I asked myself something else:
I know the second question is a bit subjective. I'm still going to use this metric because it's one that really matters to me.
To test this hypothesis, I needed an existing project. I used tsrc[4], the command-line tool we use at work[5] to manage multiple git repositories and automate GitHub and GitLab review automation.
4: https://github.com/dmerejkowsky/tsrc
I chose it because:
So here is the protocol I used:
Before I continue, I should tell you I used mypy with three important options:
Let's look at trivial patches first:
def find_workspace_path() -> Path:
head = os.getcwd()
tail = True
while tail:
tsrc_path = os.path.join(head, ".tsrc")
if os.path.isdir(tsrc_path):
return Path(head)
else:
head, tail = os.path.split(head)
raise tsrc.Error("Could not find current workspace")
This function starts by checking there is a `.tsrc` hidden directory in the working path. If not, it goes through every parent directory and check if the `.tsrc` directory is here. If it reaches the root of the file system (the second value of `os.path.split` is None), it raises an exception.
mypy did not like that `tail` started as a boolean, and then was assigned to a string.
We can fix that by using a non-empty string, with a value that reveals the intention:
- tail = True
+ tail = "a truthy string"
while tail:
An other way would be to use a type like `Union[bool,str]` but that would be more confusing I think.
Anyway, I'm not sure the quality of the code improved there. No point for mypy.
class Options:
def __init__(self, url, shallow=False) -> None:
self.shallow: bool = shallow
self.url: str = url
def save_config(options: Options):
config = dict()
config["url"] = options.url
config["shallow"] = options.shallow
with self.cfg_path.open("w") as fp:
ruamel.yaml.dump(config, fp)
We are using `save_config` to serialize a "value object" (the `Options` class) into a YAML file.
mypy saw the first two lines, `config = dict()`, `config["url"] = options.ul` and wrongly deduced that `config` was a dict from strings to strings.
Then it complained about `config["shallow"]` that was assigned to a boolean.
We can fix that by forcing the `config` type to be a dict from string to `Any`: `Any` is a "magic" type we can use precisely for this kind of situation[6].
6: http://mypy.readthedocs.io/en/latest/kinds_of_types.html#the-any-type
- config = dict() + config: Dict[str, Any] = dict()
This a bit annoying, but the type annotation makes it clearer what the `config` is. 1 point for mypy.
When you use the GitLab API, you often have to use the 'project id'. The doc says you can use the `<namespace>/<project_name>` string if it is "url encoded", like this:
Get info about the Foo project in the FooFighters namespace: GET /api/v4/projects/FooFighters%2Foo
In python, the naive approach does not work:
import urllib.parse
>>> urllib.parse.quote("FooFighters/Foo")
FooFighters/Foo
Instead you have to specify a list of "safe" characters, that is characters that you *don't* need to encode.
The doc says the default value is "/".
Thus, here's what we use in `tsrc.gitlab`:
project_id = "%s/%s" % (namespace, project) encoded_project_id = urllib.parse.quote(project_name, safe=list())
mypy saw that the default value was a string, so he complained we were using a list instead. We fixed it by:
- encoded_project_name = urllib.parse.quote(project_name, safe=list()) + encoded_project_name = urllib.parse.quote(project_name, safe="")
Here, mypy forced us to follow an implicit convention. There are two ways to represent a list of characters in Python. A real list: `['a', 'b', 'c']`, or a string: `"abc"`. The authors of the `urllib.quote()` function decided to use the second form, so it's a good thing we follow this convention too.
An other win for mypy.
We already use a lot of tools in the hope of catching bugs:
Despite of this, mypy found bugs that tests, all the linters and all the reviewers did not find.
Here are a few of them. Feel free to try and find the bug yourself, the answer will be given after the ⁂ symbol.
class GitLabAPIError(GitLabError):
def __init__(self, url: str, status_code: int, message: str) -> None:
...
def handle_stream_errors(response: requests.models.Response) -> None:
if response.status_code >= 400:
raise GitLabAPIError(
response.url, "Incorrect status code:", response.status_code)
This code wraps calls to the GitLab API, and arrange for a particular `GitLabAPIError` to be raised, when we make a "stream" request. (For instance, to download a GitLab CI artifact):
<center>⁂</center>
The problem here is that we inverted the `status_code` and `message` parameter. Easy to fix:
- raise GitLabAPIError( - response.url, "Incorrect status code:", response.status_code) + raise GitLabAPIError( + response.url, response.status_code, "Incorrect status code")
The bug was not caught because the code in question was actually copy/pasted from a CI script (and you usually don't write tests for CI scripts).
We actually don't need streamed responses anywhere in tsrc, so this is in fact dead code (and it is now gone from the code base)
def handle_json_errors(response: requests.models.Response):
try:
json_details = response.json()
except ValueError:
json_details["error"] = (
"Expecting json result, got %s" % response.text
)
...
url = response.url
if 400 <= status_code < 500:
for key in ["error", "message"]:
if key in json_details:
raise GitLabAPIError(url, status_code, json_details[key])
raise GitLabAPIError(url, status_code, json_details)
if status_code >= 500:
raise GitLabAPIError(url, status_code, response.text)
This one is slightly more interesting. It is located near the previous one and handles errors for the calls to the GitLab API which returns JSON objects.
We of course have to catch 500 errors, which hopefully do not happen often.
In case of a status code between 400 and 499, we know there was a problem in the request we made, but we need to tell the user why the request was rejected.
Most of the time the GitLab API returns a JSON object containing a `error` or `message` key, but sometimes neither key is found in the returned object, and sometimes the text of the response is not even valid JSON code.
So we have to check for both keys in the JSON object, and if not found (if we exit the for loop), just store the entire JSON response in the exception.
<center>⁂</center>
The bug is in the second `raise GitLabAPIError`. We are passing an entire object where the `GitLabAPIError` expected a string.
The fix was:
- raise GitLabAPIError(url, status_code, json_details) + raise GitLabAPIError(url, status_code, json.dumps(json_details, indent=2))
Again, this was hard to catch with tests. The case where the JSON returned by GitLab did *not* contain a `error` or `message` key only happened once in the lifetime of the project (which explain why the code was written), so manual QA and unit tests did not need to check this code path.
Anyway, note we did not blindly wrote something like `str(json_details)` to convert the JSON object to a string. We found out it was used in a message displayed to the end user, thus we use `json.dumps(json_details), indent=2)` to make sure the message contains neatly indented JSON and is easy to read.
class LocalManifest:
def __init__(self, workspace_path: Path) -> None:
hidden_path = workspace_path.joinpath(".tsrc")
self.clone_path = hidden_path.joinpath("manifest")
self.cfg_path = hidden_path.joinpath("manifest.yml")
self.manifest: Optional[tsrc.manifest.Manifest] = None
def update(self) -> None:
ui.info_2("Updating manifest")
tsrc.git.run_git(self.clone_path, "fetch")
tsrc.git.run_git(self.clone_path, "reset", "--hard", branch)
def load(self) -> None:
yml_path = self.clone_path.joinpath("manifest.yml")
if not yml_path.exists():
message = "No manifest found in {}. Did you run `tsrc init` ?"
raise tsrc.Error(message.format(yml_path))
self.manifest = tsrc.manifest.load(yml_path)
@property
def copyfiles(self) -> List[Tuple[str, str]]:
return self.manifest.copyfiles
def get_repos(self) -> List[tsrc.Repo]:
assert self.manifest, "manifest is empty. Did you call load()?"
return self.manifest.get_repos(groups=self.active_groups)
After you run `tsrc init git@example.com/manifest.git`, the manifest is cloned inside `<workspace>/.tsrc/manifest`.
Thus, the contents of the `manifest.yml` is in `<workspace>/.tsrc/manifest/manifest.yml>`.
The `LocalManifest` class represent this manifest repository.
Here's what happens when you run `tsrc sync`:
<center>⁂</center>
It's not really a bug, but mypy forced us to acknowledge that `LocalManifest.manifest` starts as None, and only gets its real value after `.load()` has been called.
We already have an `assert` in place in `get_repos()`, but mypy forced us to add a similar check in the `copyfiles` getter:
@property
def copyfiles(self) -> List[Tuple[str, str]]:
+ assert self.manifest, "manifest is empty. Did you call load()?"
return self.manifest.copyfiles
Some of the tests for tsrc are what we call end-to-end tests:
Here's how they work:
Thus, we don't have to mock file systems or git commands (which is doable but pretty hard), and things are easy to debug because in case of problem we can just `cd` to the test directory and inspect the state of the git repositories by hand.
Any way, those tests are written with a test helper called `GitServer`.
You can use this class to create git repositories, push files on some branches, and so on:
Here's what the helper looks like:
def add_repo(self, name: str, default_branch="master") -> str:
...
url = self.get_url(name)
return url
def push_file(self, name: str, file_path: str, *,
contents="", message="") -> None:
...
full_path = ...
full_path.parent.makedirs_p()
full_path.touch()
if contents:
full_path.write_text(contents)
commit_message = message or ("Create/Update %s" % file_path)
run_git("add", file_path)
run_git("commit", "--message", commit_message)
run_git("push", "origin", "--set-upstream", branch)
def tag(self, name: str, tag_name: str) -> None:
run_git("tag", tag_name)
run_git("push", "origin", tag_name)
def get_tags(self, name: str)