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 group order #3323

Merged
merged 7 commits into from Sep 12, 2016
Merged

Bugfix group order #3323

merged 7 commits into from Sep 12, 2016

Conversation

balloob
Copy link
Member

@balloob balloob commented Sep 11, 2016

Description:
As part of my PR #3203 to add reloading config to groups I introduced a bug when cleaning up the voluptuous schema validation that broke the group ordering based on their order in the config.

The problem is that passing an ordered dict through a voluptuous dictionary validator will convert it to a normal dictionary. The group component depends on the order from the config to calculate the order of groups so this caused an issue.

This PR adds a new config validator to validate ordered dictionaries while maintaining the order.

I think that we should release this as part of 0.28.1

Example entry for configuration.yaml (if applicable):

group:
  kitchen:
    name: Kitchen
    entities:
      - switch.kitchen_pin_3
  upstairs:
    name: Kids
    icon: mdi:account-multiple
    view: yes
    entities:
      - input_boolean.notify_home
      - camera.demo_camera
      - device_tracker.demo_paulus
      - group.garden

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.

@balloob balloob added this to the 0.28.1 milestone Sep 11, 2016
try:
val = value_validator(val)
except ValueError:
errors.append(vol.ValueInvalid('is not a valid value', [key]))
Copy link
Member

Choose a reason for hiding this comment

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

Should report that [val] is invalid, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, as it is the path to the value. So actually the 'key' one is not completely correct I guess. But with the patch from @kellerza that doesn't matter anymore so I'll stick with his work.

@MartinHjelmare
Copy link
Member

One comment. Changes look good to me, but I'm not too familiar with the old behavior of groups.

@kellerza
Copy link
Member

kellerza commented Sep 11, 2016

Proposal for changes here

Validating as full entries raises the correct error

voluptuous.error.MultipleInvalid: extra keys not allowed @ data['group']['travel_johann']['xname']

otherwise it does not include the group name:

voluptuous.error.MultipleInvalid: extra keys not allowed @ data['group']['xname']

@balloob
Copy link
Member Author

balloob commented Sep 12, 2016

Thanks @kellerza, I've merged your changes. I've actually simplified it a bit more. From further inspection of the voluptuous code it seems like they will only report one error at a time . So I decided to do that too.

@kellerza
Copy link
Member

Short and simple! Looks like I introduced a linting error though...

@balloob balloob merged commit 838b09b into dev Sep 12, 2016
@balloob balloob deleted the bugfix-group-order branch September 12, 2016 05:25
balloob added a commit that referenced this pull request Sep 12, 2016
* Add ordered dict config validator

* Have group component use ordered dict config validator

* Improve config_validation testing

* update doc string config_validation.ordered_dict

* validate full dict entries

* Further simplify ordered_dict validator.

* Lint fix
hartmms pushed a commit to hartmms/home-assistant that referenced this pull request Sep 17, 2016
* Add ordered dict config validator

* Have group component use ordered dict config validator

* Improve config_validation testing

* update doc string config_validation.ordered_dict

* validate full dict entries

* Further simplify ordered_dict validator.

* Lint fix
balloob added a commit that referenced this pull request Sep 21, 2016
* Add ordered dict config validator

* Have group component use ordered dict config validator

* Improve config_validation testing

* update doc string config_validation.ordered_dict

* validate full dict entries

* Further simplify ordered_dict validator.

* Lint fix
@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

3 participants