Skip to content

Update cloud auth#9357

Merged
pvizeli merged 5 commits into
devfrom
clouder
Sep 12, 2017
Merged

Update cloud auth#9357
pvizeli merged 5 commits into
devfrom
clouder

Conversation

@balloob
Copy link
Copy Markdown
Member

@balloob balloob commented Sep 10, 2017

Description:

Finish the cloud logic: register, forgot password flows.

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

Copy link
Copy Markdown
Member

@pvizeli pvizeli left a comment

Choose a reason for hiding this comment

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

Looks amazing. Only some small inputs



class UnknownError(CloudError):
"""Raised when an unknown error occurrs."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also for __init__



class CloudError(Exception):
"""Base class for cloud related errors."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that should be inside __init__

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are the class docstrings. There is no need to implement __init__ because we want to keep the base class constructor.

from .const import AUTH_FILE, SERVERS
from .util import get_mode

_LOGGER = logging.getLogger(__name__)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should have a REQUIREMENTS for botocore

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Botocore comes as part of Warrant.

raise _map_aws_exception(err)


class Auth:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know that is not needed but we use on most place class Bla(object):

return self.json(cloud.account)
@asyncio.coroutine
@_handle_cloud_errors
@RequestDataValidator(vol.Schema({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should defined the Schema not inline. Maybe in cloud/const.py or cloud/schema.py

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

but it's only used right here? Defining it in const makes it seem like we plan on re-using it, which is not the case.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I kinda like that it is defined close to where it is used, there is no need to look around for it.


@asyncio.coroutine
@_handle_cloud_errors
@RequestDataValidator(vol.Schema({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should defined the Schema not inline. Maybe in cloud/const.py or cloud/schema.py


@asyncio.coroutine
@_handle_cloud_errors
@RequestDataValidator(vol.Schema({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should defined the Schema not inline. Maybe in cloud/const.py or cloud/schema.py


@asyncio.coroutine
@_handle_cloud_errors
@RequestDataValidator(vol.Schema({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should defined the Schema not inline. Maybe in cloud/const.py or cloud/schema.py


@asyncio.coroutine
@_handle_cloud_errors
@RequestDataValidator(vol.Schema({
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should defined the Schema not inline. Maybe in cloud/const.py or cloud/schema.py

@balloob balloob mentioned this pull request Sep 22, 2017
@home-assistant home-assistant locked and limited conversation to collaborators Mar 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants