Skip to content

Stricter mypy config #5738

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Stricter mypy config #5738

wants to merge 16 commits into from

Conversation

semohr
Copy link
Contributor

@semohr semohr commented Apr 17, 2025

Description

Since we are working a lot on type hints at the moment, how about we review the available mypy configuration options and decide on a standard setup going forward?

This is meant as a discussion to align on which settings we want to adopt to improve our code quality, type safety, and overall developer experience.


Proposal

I propose enabling the following options:

allow_redefinition = true

  • Why: This allows reassigning or shadowing a name with a different type within a function or scope. It’s helpful for code clarity, especially in cases like variable narrowing or reusing loop variables.
  • Example:
    x: int = 1
    x: str = "foo"  # Without this flag, mypy would complain

check_untyped_defs = true

  • Why: This tells mypy to check the bodies of functions even if they don’t have type annotations. It helps catch errors early, even in partially typed code.
  • Impact: Encourages typing functions over time while still providing coverage for legacy code.

disallow_incomplete_defs = true

  • Why: Disallows defining functions with partial type annotations (including arguments). This enforces consistency and helps ensure that function if they are typed, are typed fully.
  • Example:
    def f(x : int, y):  # This would error without a type for y and return type
        ...

disallow_any_generics = true

  • Why: Requires specifying type parameters for generic types like List, Dict, etc. Avoids untyped containers which can mask bugs or make type inference harder.
  • Example:
    x: list = []  # Error
    x: list[int] = []  # OK

disallow_untyped_decorators = true

  • Why: Ensures that decorators are properly typed. Untyped decorators can cause the function they decorate to become untyped too, making it harder for mypy to validate code.
  • Impact: Should help with typing pipelines where we heavily use decorators.

Me can even go stricter (e.g. disallow_untyped_calls, warn_unused_ignores) or more lenient (e.g. leave check_untyped_defs off). I think the proposed setup strikes a good balance between safety and practicality.

Would love to hear thoughts or additions!


This is based on the #5701 PR.

@wisp3rwind
Copy link
Member

I'll try to look at this in more detail at some point (right now, I'm not very familiar with most of mypy's options).

In general, in my opinion, we should eventually set most of these. Probably rather sooner than later. For example, when looking at your PR #5701, I realized that almost all callers of the plugin interface are not in fact typed right now. Thus, we get very little feedback from mypy at this point.

I'm uncertain about allow_redefinitions: I've definitely worked around a few of these already in beets. On the other hand, maybe in some cases that actually helped to improve clarity? I'll have to look around for those cases to get an idea. I feel like this might behave differently in Python than in, say, Rust, where shadowing would require an explicit let ... statement.

Last time I ran mypy with --check-untyped-defs, the output was still a bit overwhelming. Maybe let's wait for the current wave of typings PRs to be merged, and then review? I'd also argue to get these in one by one if they require changes: In my experience (and I'm also guilty of this), one big factor that leads to stalling PRs for beets has been when too many things were done in one go.

@semohr
Copy link
Contributor Author

semohr commented Apr 18, 2025

You're right that most plugins don’t fully leverage type hints yet. Before merging additional typing-related PRs like #5734 or #5716, I thought it might be worth configuring some stricter mypy options now. That way, we can avoid having to do another round of refactors later just to adapt to those settings.

I realize enabling more options—like --check-untyped-defs—can make a lot of existing issues visible all at once, which can definitely feel overwhelming. But we don’t have to fix everything immediately, especially since mypy isn't currently blocking PRs. In some cases, we could even ignore certain new errors temporarily.

Looking ahead, I think it's useful to start moving toward these stricter checks for new code, even if some legacy code doesn’t yet meet the same standard. It's okay if some parts lag behind a bit as long as we keep improving incrementally.

@wisp3rwind
Copy link
Member

We need to take #5728 or #5745 into account whenever we do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants