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

Bugfix: Input Datatime config schema #12552

Merged
merged 2 commits into from
Feb 20, 2018
Merged

Bugfix: Input Datatime config schema #12552

merged 2 commits into from
Feb 20, 2018

Conversation

cdce8p
Copy link
Member

@cdce8p cdce8p commented Feb 20, 2018

Description:

Has_at_least_one_key_value only works if parameter are optional

Example entry for configuration.yaml (if applicable):

input_datetime:
  both_date_and_time:
    name: Input with both date and time
    has_date: true
    has_time: true
  only_date:
    name: Input with only date
    has_date: true
  only_time:
    name: Input with only time
    has_time: true

Checklist:

  • The code change is tested and works locally.

* Has_at_least_one_key_value only works if parameter is optional
@cdce8p cdce8p requested a review from a team as a code owner February 20, 2018 13:09
Copy link
Contributor

@tinloaf tinloaf 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 see how has_at_least_one_key_value should only work on optional arguments, but of course these could be made optional.

@@ -43,8 +43,8 @@
DOMAIN: vol.Schema({
cv.slug: vol.All({
vol.Optional(CONF_NAME): cv.string,
vol.Required(CONF_HAS_DATE): cv.boolean,
vol.Required(CONF_HAS_TIME): cv.boolean,
vol.Optional(CONF_HAS_DATE): cv.boolean,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please set the default to False. Otherwise, None is being passed around instead of False, which of course works just as False in if self._has_date, but gives you strange values in the attributes. Also, yay for type safety.

@cdce8p
Copy link
Member Author

cdce8p commented Feb 20, 2018

@tinloaf It would work on required parameters as well, but than voluptuous catches it first if not given.

@tinloaf
Copy link
Contributor

tinloaf commented Feb 20, 2018

I just saw that https://home-assistant.io/components/input_datetime/ already listed the parameters as being optional. Thanks for catching this!

@tinloaf tinloaf merged commit 7829e61 into home-assistant:dev Feb 20, 2018
@cdce8p cdce8p deleted the bugfix-input-datetime branch February 20, 2018 15:34
@balloob balloob mentioned this pull request Feb 22, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants