Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Move mypy configuration into pyproject.toml #11315

Closed
wants to merge 1 commit into from

Conversation

callahad
Copy link
Contributor

This is mainly to take advantage of the significantly more convenient
syntax for module overrides. For example, instead of:

[mypy-synapse.api.*]
disallow_untyped_defs = True

[mypy-synapse.app.*]
disallow_untyped_defs = True

[mypy-synapse.crypto.*]
disallow_untyped_defs = True

The pyproject.toml file allows:

[[tool.mypy.overrides]]
module = [
    "synapse.api.*",
    "synapse.app.*",
    "synapse.crypto.*",
]
disallow_untyped_defs = true

Not a huge difference with two or three modules, but we list many more.

This is especially important as I'm about to set disallow_untyped_defs
at the global level and provide module-level opt-outs so that we require
typed definitions in all newly added code.

Switching to this format will save several hundred lines of repetitive
boilerplate and clarify the intent of each stanza.

It's also easier to keep things ordered, since it works to just pipe a
region to sort, while mypy.ini requires handling a 3-line structure.
Ordering helps keep these settings synced up with the files on disk.

Signed-off-by: Dan Callahan [email protected]

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@callahad callahad requested a review from a team as a code owner November 11, 2021 19:53
@callahad
Copy link
Contributor Author

Split out from #11302

@callahad
Copy link
Contributor Author

callahad commented Nov 11, 2021

cc: @clokep 👋

Understanding your objections about independent tools Voltron-ing themselves into pyproject.toml, but (a) it seems to be a clear part of the current zeitgeist, so limited gains from being contrarian while there are maintenance gains at least for the duration of our mypy-improvement sprint, and (b) once we get things slimmed down to where the mypy config is just couple dozen lines, we can move back to mypy.ini if we really want to.

This is mainly to take advantage of the significantly more convenient
syntax for module overrides. For example, instead of:

    [mypy-synapse.api.*]
    disallow_untyped_defs = True

    [mypy-synapse.app.*]
    disallow_untyped_defs = True

    [mypy-synapse.crypto.*]
    disallow_untyped_defs = True

The pyproject.toml file allows:

    [[tool.mypy.overrides]]
    module = [
        "synapse.api.*",
        "synapse.app.*",
        "synapse.crypto.*",
    ]
    disallow_untyped_defs = true

Not a huge difference with two or three modules, but we list many more.

This is especially important as I'm about to set disallow_untyped_defs
at the global level and provide module-level opt-outs so that we require
typed definitions in all newly added code.

Switching to this format will save several hundred lines of repetitive
boilerplate and clarify the intent of each stanza.

It's also easier to keep things ordered, since it works to just pipe a
region to `sort`, while mypy.ini requires handling a 3-line structure.
Ordering helps keep these settings synced up with the files on disk.

Signed-off-by: Dan Callahan <[email protected]>
@callahad
Copy link
Contributor Author

Comparison between next steps with pyproject.toml and mypy.ini at https://gist.github.com/callahad/596d9e264f08179dcc2b1997ef8ce375

  • mypy.ini: -90,+814
  • pyproject.toml: -48,+288

@clokep
Copy link
Member

clokep commented Nov 12, 2021

Not a huge difference with two or three modules, but we list many more.

#11322 aims to cut this down quite a bit. As far as I can tell we only need to add ~10 more to get the entire code-base covered. The big stopping point would be synapse.util.

It's also easier to keep things ordered, since it works to just pipe a region to sort, while mypy.ini requires handling a 3-line structure. Ordering helps keep these settings synced up with the files on disk.

I don't understand this part. Ordering isn't a huge deal when we're adding individual lines?

(a) it seems to be a clear part of the current zeitgeist

I do not agree, I think this is really a preference thing and not a clear-cut movement.

so limited gains from being contrarian while there are maintenance gains at least for the duration of our mypy-improvement sprint, and (b) once we get things slimmed down to where the mypy config is just couple dozen lines, we can move back to mypy.ini if we really want to.

This sounds to me like churn for a small benefit for a small period of time.

I'm not going to block this PR, but I do not agree it is an improvement.

@reivilibre
Copy link
Contributor

Personally I like the TOML syntax more than the weird INI variant that Python tools seem to use, though I do also like single-use files (especially because seeing mypy.ini means I don't have to wonder if it's in pyproject.toml or setup.cfg (as some tools are), etc. I had a quick look and don't think you can have both in this case.

Further, multi-module overrides in standard syntax seem to be specified like [mypy-synapse.abc,synapse.def,…] which does not seem nice when you have so many to exclude. I prefer a 1-entry per line list — shame there's no equivalent in the evil INI format nor the ability to have mypy.toml.

With the above said, I would personally prefer what Dan is suggesting. That said, if someone feels quite against dropping mypy.ini then I wouldn't insist.

(the 3-line stanzas would be OK imo, noisy but since our job is to remove them rather than think about adding new ones, it would certainly be an improvement)

@callahad
Copy link
Contributor Author

callahad commented Nov 12, 2021

My read on the situation is that Clokep is skeptical of the value of pyproject.toml, but not so opposed as to block it. Reivilibre is generally positive toward the proposal, but wishes mypy supported TOML syntax in a standalone file. As the proposer, I'm naturally in favor of this pull request.

In the absence of someone raising a blocking objection, I'd like to request a review with a goal of merging by end of day. This will bitrot quickly as work is done in typing, and the specific implementation here is highly constrained: it's either our current mypy.ini or this pyproject.toml.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

I'm going to trust @callahad that the two sets of config are equivalent, and consider the spirit of the PR rather than the letter of it.

Broadly, I have no strong opinion on whether we put stuff in mypy.ini or pyproject.toml; I think there is for and against for both systems. If @callahad is keen, I'll not stand in his way.

Better fix up the conflict though :/

@@ -0,0 +1 @@
Port mypy.ini into pyproject.toml.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Port mypy.ini into pyproject.toml.
Move mypy configuration settings into `pyproject.toml`.

@clokep
Copy link
Member

clokep commented Nov 29, 2021

My read on the situation is that Clokep is skeptical of the value of pyproject.toml, but not so opposed as to block it. Reivilibre is generally positive toward the proposal, but wishes mypy supported TOML syntax in a standalone file. As the proposer, I'm naturally in favor of this pull request.

Yes, I'd say I'm a -0.5 on it, if you'd like me to use that system. 😄

@clokep
Copy link
Member

clokep commented Jun 1, 2022

I'm going to close this since it has bitrotted. We can re-do this in the future if we want.

@clokep clokep closed this Jun 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants