Add type hints to homeassistant.auth #15853
Conversation
ghost
added
the
|
I am not sure we are in a good position to refactor auth since it is still under active development. What is our strategy for typing stuff? |
|
I'm not sure I understand what you mean by refactoring -- in my opinion the changes this PR don't really do that (with maybe the change to use namedtuple instead of dict for user meta being an exception). Anywat, I think having typing there while it's in active development would be especially beneficial, as it can prevent some issues before they reach end user systems. That's the reason I picked auth as the first thing to extend type hint coverage to. But sure, hearing what the overall strategy and desirability for typing stuff in the project would be cool. Personally, I would be willing to help extend it. I actually already have some more work done on that front that I haven't submitted yet, would like to get this one out of the way first. |
|
Auth is under "active" development means I have several PRs in the pipe line, and few others haven't open PR yet. Accepting your PR means lots of merging I need to deal with. For the typing in general, I don't know what other people thinking, but I personally like a parameter has typing when its type is some Class, but not a dict or list. For example async def auth_manager_from_config(hass, provider_configs)I like change it to async def auth_manager_from_config(
hass: HomeAssistant, provider_configs) -> AuthManagerBut not your version async def auth_manager_from_config(
hass: HomeAssistant,
provider_configs: List[Dict[str, Any]]) -> AuthManagerAnyway, I suggest you open an issue in https://github.com/home-assistant/architecture/issues to collect more input on this topic. |
|
I think that right now because we have so many PRs open, adding types to it would be a pain for merge conflicts. Once the dust has settled a bit, I do want to have types on the auth code. |
|
On Tue, Aug 7, 2018 at 8:29 PM, Jason Hu ***@***.***> wrote:
Auth is under "active" development means I have several PRs in the pipe
line, and few others haven't open PR yet. Accepting your PR means lots of
merging I need to deal with.
Sure, but it's one time only. And I'm sure it's possible to arrange a
moment that causes least churn.
For the typing in general, I don't know what other people thinking, but I
personally like a parameter has typing when its type is some Class, but not
a dict or list.
FWIW I disagree with this -- if type hints are desirable in the first
place, I don't know why one wouldn't use them accurately.
Anyway, I suggest you open an issue in https://github.com/home-
assistant/architecture/issues to collect more input on this topic.
Done.
|
|
Having type hints there already now would be beneficial for the ongoing work. At the extreme other end, for code that is ready and not being developed and there are no new things are interfacing with it, it doesn't matter much at all whether there are type hints or not. The earlier they are introduced, the more useful they are. |
homeassistant/auth/__init__.py
Outdated
| @@ -47,22 +47,24 @@ | |||
| class AuthManager: | |||
| """Manage the authentication for Home Assistant.""" | |||
|
|
|||
| def __init__(self, hass, store, providers): | |||
| def __init__(self, hass: HomeAssistant, store: auth_store.AuthStore, | |||
| providers: Dict[Tuple[str, Optional[str]], AuthProvider]) \ | |||
There was a problem hiding this comment.
Maybe create a type alias for Dict[Tuple[str, Optional[str]], AuthProvider]?
homeassistant/auth/models.py
Outdated
| id = attr.ib(type=str, default=attr.Factory(lambda: uuid.uuid4().hex)) | ||
| is_owner = attr.ib(type=bool, default=False) | ||
| is_active = attr.ib(type=bool, default=False) | ||
| system_generated = attr.ib(type=bool, default=False) | ||
|
|
||
| # List of credentials of a user. | ||
| credentials = attr.ib(type=list, default=attr.Factory(list), cmp=False) | ||
| credentials = attr.ib(type=list, default=attr.Factory(list), cmp=False) # type: List[Credentials] # noqa: E501 pylint: disable=line-too-long |
There was a problem hiding this comment.
Avoid line-too-long where possible.
Here I think you can break after (
|
We're now at a point where I don't expect any major changes to happen anymore. |
|
Rebased against dev. |
| @@ -173,11 +198,17 @@ def __init__(self, hass): | |||
| if 'jwt_key' not in rt_dict: | |||
| continue | |||
|
|
|||
| created_at = dt_util.parse_datetime(rt_dict['created_at']) | |||
| if created_at is None: | |||
There was a problem hiding this comment.
When homeassistant.util.dt.DATETIME_RE does not match the input. Probably not that all often in practice, but parse_datetime can return None and is declared to do so by using Optional, so we need to appease type checks and prevent assigning the possible None to (the non-Optional) RefreshToken.created_at.
There was a problem hiding this comment.
We should probably change it to raise value error. Fail loudly
* Always load users in auth store before use * Use namedtuple instead of dict for user meta * Ignore auth store tokens with invalid created_at * Add type hints to homeassistant.auth
Description:
Adds type hints to homeassistant.auth.
Checklist:
tox. Your PR cannot be merged unless tests passIf user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py..coveragerc.If the code does not interact with devices: