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.

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

Sydent typing, part 1 #1123

Merged
merged 10 commits into from
Dec 3, 2021
Merged

Sydent typing, part 1 #1123

merged 10 commits into from
Dec 3, 2021

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Nov 26, 2021

First chunk of #1110. Forward references to #1124 and #1125.

I tentatively set the date to the coming Friday, 3rd Dec.

Preview: https://pr1123--matrix-org-previews.netlify.app

-> https://pr1123--matrix-org-previews.netlify.app/blog/2021/12/03/type-coverage-for-sydent-motivation

This was referenced Nov 26, 2021
Copy link

@squahtx squahtx left a comment

Choose a reason for hiding this comment

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

spotted some small typos!

Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com>

We might also have been able to detect the error by looking at how we used `sid`. Unfortunately, the only use of was in a conversion `str(sid)`, which is a perfectly type-safe call for both `sid: int` and `sid: Awaitable[int]`. But let's put that aside for a second—suppose we added `"sid": sid` directly into the `resp` dictionary. Could mypy have spotted _that_?

Unfortunately not. Because `resp` has no annotation, we have to rely on how it's used to assign it a type. And it's only really used as a return value. What's the function's annotation?
Copy link

Choose a reason for hiding this comment

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

I got very slightly confused at "What's the function's annotation?" Maybe add something clarifying that you are talking about the function that returns resp, i.e. something like "What's the annotation of the function that returns resp?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0854de6 tries to make this clearer---what do you think?

Copy link

Choose a reason for hiding this comment

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

Perfect!

@H-Shay
Copy link

H-Shay commented Dec 1, 2021

Very, very cool and well-written/clear. Thanks for doing this! I saw one or two very small things that I commented on, let me know if anything is unclear.

David Robertson and others added 3 commits December 1, 2021 11:15
- discuss the checks that `mypy` provides; and finally
- reflect on the state of Python's typing ecosystem.

In this first part, we'll concentrate on the first two topics; the [second](2021-12-10-sydent-typing-2.mdx) covers the middle two; and the [third](2021-12-17-sydent-typing-3.mdx) the last two.
Copy link

Choose a reason for hiding this comment

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

These will 404 at time of publication...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hoping to be lazy. d6a48bc does it properly.

@callahad
Copy link

callahad commented Dec 2, 2021

Looks great! I wish there was a way to discuss the Sydent failures without making us look quite so bad at monitoring, but hey, there you go (Sydent being a somewhat unloved project means these things get missed, but also means that static analysis is that much more valuable.)

Similarly, "we weren't running mypy in CI" maaaay do with a clarification that we weren't running it in Sydent's CI, but were in Synapse? Maybe. But this is great, thank you for writing it.


[Sydent](https://github.com/matrix-org/sydent) is the reference Matrix Identity server. It provides a [lookup service](https://matrix.org/docs/spec/identity_service/r0.3.0), so that you can find a Matrix user via their email address or phone number (if they've chosen to share it).

We recently worked on [improving Sydent's type coverage](https://github.com/matrix-org/sydent/issues/414). Type coverage (or "typing") refers to annotations in Python source code that specify the type of an expression. These annotations are entirely optional, but if present, they allow tools to reason about the types that flow through a program. One popular tool is [`mypy`](https://mypy.readthedocs.io/en/stable/), which can often use the type information to spot bugs before you ship them! We worked to make Sydent pass `mypy --strict`. It now does so, with an overall precision of 94%! Here's what that process looks like:
Copy link

Choose a reason for hiding this comment

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

You say "type" a lot in this paragraph. Can it be cut down?

Type coverage (or "typing") refers to annotations in Python source code that specify the type of an expression.

Close to being a self-referential definition ("typing refers to specifying types")...

As a rough sketch, maybe something along the lines of:

We recently worked on improving Sydent's type coverage: the proportion of the codebase with explicit annotations denoting the kind of data that each variable, parameter, and return value is expected to hold. These annotations are optional, but when they're present tools like mypy can use them to analyze your code and prevent entire classes of errors before you ship them! In this instance we attempted to make Sydent pass mypy in "strict" mode, which it now does. Here's what the process looked like:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've nicked most of this and applied a little tweaking in dac5e78.

- Thoughts
- Tech
author: David Robertson
---
Copy link

Choose a reason for hiding this comment

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

Maybe throw a little:

This is the first of three blog posts on improving type coverage in Sydent. Check back next Friday for another installment!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is 740800b.

@DMRobertson
Copy link
Contributor Author

Similarly, "we weren't running mypy in CI" maaaay do with a clarification that we weren't running it in Sydent's CI, but were in Synapse? Maybe. But this is great, thank you for writing it.

That's fair. For completeness, 42c268a.

@DMRobertson DMRobertson merged commit 2922139 into master Dec 3, 2021
@DMRobertson DMRobertson deleted the dmr/sydent-typing-1 branch December 3, 2021 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants