Don't ignore mypy errors by default

[KapJI]

Proposed change

Motivation

Currently mypy checks only strictly typed parts of HA, everything else is just ignored.
This results that many components have type hints which are not checked and therefore completely useless.

Solution

The idea is ignore errors in components which are currently broken and allow developers of these components to fix them and remove their component from ignored list. Even if they're not ready yet to enable strict checks.

This is while keeping components which are not broken in that state by checking their type hints.

If new component will have any type hints added they will be also checked. If new type hints are added, they should be checked, otherwise what's the point of having broken type hints?

This will allow to improve code by gradually fixing what is currently broken and doesn't satisfy its own type hints.

Implementation

New hassfest plugin which generates and validates mypy config based on python list. Move mypy config from setup.cfg to mypy.ini.

Components which do not want to have strict checks enabled are now listed in .no-strict-typing file in the repo root which is easier to maintain. hassfest will use it for generating mypy config.

Strict checks are now enabled by default and this covers all core modules. New core modules will be automatically strictly checked.

Current errors

Currently there are 3258 errors in 599 files in 238 components. Please note this is not with strict enabled, it's just checking existing type hints. With strict mode fully enabled there're 44k errors.

• Full list of errors.
• List of ignored components. There are many generic components like camera, config, climate, device_tracker, fan, recorder, script, etc. They probably should be fixed first.

Strict mode

I disabled strict = true because it's applied globally and it's an equivalent of mypy --strict. Instead I enabled the same checks manually. From mypy --help:

  --strict                  Strict mode; enables the following flags: --warn-unused-configs, --disallow-any-generics, --disallow-subclassing-any, --disallow-untyped-calls, --disallow-untyped-defs,
                            --disallow-incomplete-defs, --check-untyped-defs, --disallow-untyped-decorators, --no-implicit-optional, --warn-redundant-casts, --warn-unused-ignores, --warn-return-any, --no-
                            implicit-reexport, --strict-equality

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[probot-home-assistant]

Hey there @Julius2342, @farmio, @marvin-w, mind taking a look at this pull request as its been labeled with an integration (knx) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[probot-home-assistant]

Hey there @home-assistant/core, mind taking a look at this pull request as its been labeled with an integration (automation) you are listed as a codeowner for? Thanks!
_{^{(message by CodeOwnersMention)}}

[bdraco]

@KapJI CI will fail due to #49271

[KapJI]

Ok, how about this one?

Python lists should be much easier to maintain and it doesn't require putting an extra file in the repo root.
Also this keeps all mypy configuration in the single place.

[KapJI]

@frenck what do you think?

[KapJI]

@frenck friendly ping. New broken type annotation are added almost every day, let's secure the current state at least.

@cdce8p I think you also may be interested in this.

[KapJI]

@MartinHjelmare maybe you have any feedback on this?

[KapJI]

Now it generates mypy.ini based on new field in manifest.json.

Because of that I removed these modules from strictly typed list, with this new model the whole component should be strictly typed to enable strict checks:

homeassistant.components.recorder.purge
homeassistant.components.recorder.repack
homeassistant.components.sonos.media_player

Also I added check that component which is marked with strictly_typed is not in ignored list, otherwise ignored list will override strict checks.

I tested that strict checks are active in core modules and components which are marked with strictly_typed. So it works as expected.

[frenck]

This should not be part of the manifest. There is nothing at runtime that needs this.

[KapJI]

@frenck what do you think about previous version?

@MartinHjelmare your opinion?

[MartinHjelmare]

I think it can be part of the manifest. We have codeowners already there which is a similar problem.

[KapJI]

I think having this is the manifest and in documentation will motivate developers to add type hints to new integrations since the beginning. And existing integrations are more likely to be updated.

Otherwise they need to figure out how this config is generated and update hassfest plugin which is in general more difficult and less clear step.

[KapJI]

@frenck is this what you meant?

[frenck]

Yeah, awesome!

Now, we can start reducing this list and prevent (well, encourage) new entries from being added 👍

[KapJI]

I rebased to current dev and mypy passes locally, should be safe to merge it now.

I'd suggest to be careful with merging PRs which are not rebased on top of this as they can easily add broken type hints.

[frenck]

I'd suggest to be careful with merging PRs which are not rebased on top of this as they can easily add broken type hints.

Those will cause build failures, which can be annoying, but not problematic :)

[frenck]

Awesome work @KapJI! 🎉

I've adjusted our developer documentation to provide information around this as well.

Let's hope we'll see more nicely typed things from this! (Oh... and as it seems my integrations are in the naughty list 😢)

[balloob]

We should not make new integrations require opt-out of type checking. This severely increases the barrier of entry to submit integrations to Home Assistant.

I was helping someone today. They had made a PyPI package and scaffolded an integration. Pre-commit failed because a) new functions he added didn't have the typing and b) their PyPI package didn't have types and got called from a typed context. The mypy output is not straight forward to fix, and without the ability to commit, they can't even push up their work to ask for help (they didn't know about --no-verify).

This move will cause more integrations to go towards HACS, bypassing our review completely. If setting up people for a technically perfect contribution means we don't get contributions… we miss our goal. Having people go to HACS is worse as our reviewers catch a lot + we add more checks all the time that improve code quality of existing integrations.

We're open source and we need to keep the environment welcoming to attract new developers. Allowing for opt-in type checking allows new developers to grow as they learn more. We should not expect new developers to play the game at expert level out of the gate.

[frenck]

I am a believer of default best practices, which including typing.

The only thing I cannot understand is how this is any different compare to testing? We require those by default, or else coverage will fail. Is that wrong too?

[bdraco]

How about changing the scaffolding to ask them if they want to opt-in?

[balloob]

We don't require tests for most part of the integration. We only require it for config flow because we have deemed it so important that adding that burden was worth it. However, we did make it as easy as possible to have all the required tests be generated based on the integration that is scaffolded.

If you have incorrect types you can't commit your code. That is very frustrating. And the problem is that these are contributors that we won't hear about, because they quit.

A better approach is to make it part of the integration quality scale. I suggest we make it part of the gold level, which also introduces requirements for testing. Since quality scale is part of the manifest, we can even enforce this via hassfest.

[Danielhiversen]

Should we (always) require that the underlying library supports typing?
#49712
(I hope not, or maybe only for gold level)

[balloob]

I don't think that Home Assistant should reject dependencies for lack of typing.

We should all not forget where we came from. Being a new programmer, playing with Python, trying to control your home with a couple of key strokes. We shouldn't try to just attract most senior Python programmers that have been exposed to typing at their work. We're open source and we should focus on being accessible to even beginning Python programmers.

When we help a person learn Python, the odds that they stay around are a lot higher than some senior dev passing by to do a contribution. We have and had many core contributors that learned Python and/or working on big projects because of Home Assistant. Those people are amazing.

(Note, discussion is about integrations, not the core)

Let's make type checking opt-in for new integrations and let's add it as a requirement of the gold level of the integration quality scale.

[frenck]

We don't require tests for most part of the integration

That is no different in this case. We indeed don't require it, but the CI will fail, unless you explicitly add stuff to coveragerc.

This method is no different in that regard.

If you have incorrect types you can't commit your code. That is very frustrating.

That I agree on! But, isn't that solved by giving a proper error message? with e.g. a friendly instruction that says, either add typing or add it to the list?

The list has a additional big a advantage; it provides a to-do list (instead of a done list). It can be slimmed down.

[frenck]

Should we (always) require that the underlying library supports typing?
#49712
(I hope not, or maybe only for gold level)

We cannot do that, but we could require it maybe for certain QA scales. Same as with async or test requirements.

However, adding typing to an integration when upstream package doesn't provide it, is hard in general an not and limition of anything Home Assistant. You'll probably need to cast and ignore a lot to get something like that even remotely correct or helpful.

[balloob]

The problem with the error message is that it's raised by pre-commit and so disconnected from scaffolding/hassfest.

[KapJI]

There are several difficulties with linking typing and gold level:

1. Some existing integrations with gold level may not pass it, we will needed to downgrade them unless they can be quickly fixed.
2. It will be harder or not possible to enable strict checks if integration doesn't satisfy other requirements for gold level.

Regarding pre-commit: it's possible to run mypy from some wrapper which will check if integration is added in config and will provide clear instructions how to add it there if person is not ready yet to fix these errors. I've seen something similar in urllib3.

Also it can show warning if integration is excluded from type checking at all (added to IGNORED_LIST). This should help to get these integrations with broken type annotations to be fixed sooner.

And about friendliness to newcomers. From my personal experience you can attract more people to work on the project if you just try to be more nice and polite when people are sending their contributions. For me friendly environment where my contributions are welcomed is much bigger factor for my desire to contribute more than solving technical roadblocks like adding an extra line to some config. I really believe you can do this.

[frenck]

It will be harder or not possible to enable strict checks if integration doesn't satisfy other requirements for gold level.

This should not be coupled with the "qa_scale" manifest status for deciding to check yes/no. Even an integration without any rating (for whatever reason), should just be able to opt-in/out regardless of the scale.

QA scale is a manual process at this point.

I've been playing with the stuff for a bit, I agree this will add annoyance for newcomers. As a matter of fact, I think our core lacks typing too much typing to make it even part of any scale at this point.

@KapJI Let's revert 2a0cf73 in a PR and let us move forward from that point on. If the foundation is solid, it might be more worth flipping it again in the future. Right now, it might indeed be too soon to follow the ideal path on this.

(Meh 😞 )