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

Mark homeassistant PEP 561 compatible #28866

Closed
wants to merge 1 commit into from
Closed

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Nov 18, 2019

Description:

Mark homeassistant package as pep 561 compatible https://www.python.org/dev/peps/pep-0561/. This allow custom components to run mypy checks on references to homeassistant components.

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist

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

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

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

If the code does not interact with devices:

  • Tests have been added to verify that the new code works.

@probot-home-assistant probot-home-assistant bot added core small-pr PRs with less than 30 lines. labels Nov 18, 2019
@pvizeli pvizeli requested a review from scop November 19, 2019 12:45
Copy link
Member

@scop scop left a comment

Choose a reason for hiding this comment

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

I don't think we can do this yet. PEP 561 says about the marker file:

if a top-level package includes it, all its sub-packages MUST support type checking as well.

A lot of our stuff isn't type checked at all yet, at least those parts I think don't qualify. And a lot have inline mypy exclusions, I suppose those don't qualify either.

https://www.python.org/dev/peps/pep-0561/#packaging-type-information

@elupus
Copy link
Contributor Author

elupus commented Nov 19, 2019

We run mypy internally on all of home assistant. So at least it doesn't fail.
Also home assistant isn't really a lib per say. So this is mostly for custom components, so if it is somewhat incomplete I don't think it matters too much. I don't think anything could get worse by this?

I'm not too vested in this though.

@scop
Copy link
Member

scop commented Nov 21, 2019

I'm fairly certain (but haven't tested) that if we mark ourselves as typed but we actually are not typed everywhere, things will get noticeably worse for custom components and other "external" users that happen to use the parts that are not type hinted.

An example: ruamel.yaml does exactly this, marks itself as PEP 561 compatible but lacks severely in type hints. Because of that we have to add some # type: ignores to get mypy checks to pass when ruamel.yaml is installed, but that will cause errors when it isn't (because we're running with warn_unused_configs on and that's a good default to keep codebase clean of unnecessary type ignores). And further because of this we have several mypy pre-commit configurations. So it's definitely worse. See https://bitbucket.org/ruamel/yaml/pull-requests/42

@scop
Copy link
Member

scop commented Nov 21, 2019

So at the minimum I'd say someone needs to test how type checks behave if we have that marker, and an external thing uses some of our untyped functionality and runs type checks with the same strict config as we do. The original report says the change is tested, but is it tested against this use case?

@elupus
Copy link
Contributor Author

elupus commented Nov 21, 2019

It's just tested against my custom component. It is not set especially strict thought.

@elupus
Copy link
Contributor Author

elupus commented Nov 21, 2019

The problem is that mypy complains whenever home assistant component are imported that it can't be found. since it consider the lib untyped. So it reduced warnings by about 30 in the custom component.

@scop
Copy link
Member

scop commented Nov 21, 2019

Is your component using any of our untyped functions?

@elupus
Copy link
Contributor Author

elupus commented Nov 21, 2019

I'm unsure. The component isn't fully typed either. https://github.com/elupus/hass_nibe

I will try to do some more verification.

@springstan
Copy link
Member

@elupus any updates on this PR? :)

@elupus
Copy link
Contributor Author

elupus commented Feb 12, 2020

Sorry no. Not had time to look at it.

@springstan
Copy link
Member

No worries, I was just curious.

@stale
Copy link

stale bot commented Mar 21, 2020

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

@stale stale bot added the stale label Mar 21, 2020
@balloob balloob closed this Mar 25, 2020
@lock lock bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed core small-pr PRs with less than 30 lines. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants