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

Component setup error messages with markdown #3919

Merged
merged 2 commits into from Nov 3, 2016

Conversation

kellerza
Copy link
Member

Description:
This is an improvement on #3738 to log persistent_notification error messages in more locations in bootstrap and include markdown.

The messages have been simplified to only show a single card, instead of the original two cards. If a platform or component exist a link will be added to the home-assistant.io website.

Markdown support

@balloob should I clean up the _PERSISTENT_ERRORS var on HOME_ASSISTANT_START?

CC: @justweb1

Related issue (if applicable): fixes #

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.

@mention-bot
Copy link

@kellerza, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @fabianhjr and @persandstrom to be potential reviewers.

link: Optional[bool]=False):
"""Print a persistent notification."""
_PERSISTENT_ERRORS[component] = (_PERSISTENT_ERRORS.get(component, False)
or link)

Choose a reason for hiding this comment

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

farcy v1.1

  • 38: W503 line break before binary operator

_PERSISTENT_ERRORS[component] = _PERSISTENT_ERRORS.get(component) or link
_lst = [HA_COMPONENT_URL.format(name) if link else name
for name, link in _PERSISTENT_ERRORS.items()]
message = ('The following components and platforms could not be set up:\n'
Copy link
Member

Choose a reason for hiding this comment

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

If we're using markdown, why not use list syntax for better readability.

Copy link
Member

Choose a reason for hiding this comment

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

Another tip: add a link at the end to /dev-info so they can see the validation errors.

[See validation errors](/dev-info)

@balloob
Copy link
Member

balloob commented Oct 18, 2016

This is ok to merge when the Polymer support has been merged and comments addressed 👍

@balloob balloob self-assigned this Oct 18, 2016
@justweb1
Copy link
Contributor

justweb1 commented Oct 18, 2016

@kellerza
home-assistant/frontend#128 comments have been addressed and has been merged.

@kellerza
Copy link
Member Author

Comments addressed. Unfortunately I can't successfully test it yet.

Feedback on gitter...

@kellerza
Copy link
Member Author

kellerza commented Nov 2, 2016

For now I have removed the relative /dev link, but list syntax supported correctly

@kellerza kellerza changed the title WIP: Component setup error messages with markdown Component setup error messages with markdown Nov 2, 2016
@balloob balloob merged commit 0d14920 into home-assistant:dev Nov 3, 2016
@kellerza kellerza deleted the bootstrap_notify branch November 3, 2016 04:05
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants