8000 Use voluptuous for mysensors by MartinHjelmare · Pull Request #2992 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use voluptuous for mysensors #2992

8000
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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 27, 2016
Merged

Use voluptuous for mysensors #2992

merged 1 commit into from
Aug 27, 2016

Conversation

MartinHjelmare
Copy link
Member
@MartinHjelmare MartinHjelmare commented Aug 26, 2016 •
edited
Loading

Description:

  • Add voluptuous config validation for mysensors
  • Remove and clean up parts that are not needed for pymysensors 0.7.

Question:
@fabaff Do we need to add something to mysensors platforms that are not configured individually? All configuration for mysensors is done through the component.

Related issue (if applicable):
#2800

Example entry for configuration.yaml (if applicable):

mysensors:
  gateways:
    - device: '/dev/ttyUSB0'
      persistence_file: 'path/mysensors.json'
      baud_rate: 38400
    - device: '/dev/ttyACM0'
      persistence_file: 'path/mysensors2.json'
      baud_rate: 115200
    - device: '192.168.1.18'
      persistence_file: 'path/mysensors3.json'
      tcp_port: 5003
    - device: mqtt
      persistence_file: 'path/mysensors4.json'
      topic_in_prefix: 'mygateway1-out'
      topic_out_prefix: 'mygateway1-in'
  debug: true
  optimistic: false
  persistence: true
  retain: true
  version: 2.0

Checklist:
If code communicates with devices, web services, or a:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@fabaff
Copy link
Member
fabaff commented Aug 26, 2016

Do we need to add something to mysensors platforms that are not configured individually? All configuration for mysensors is done through the component.

I don't think so. PREVENT_EXTRA is the default which should help us to catch unwanted configuration entries.

@MartinHjelmare
Use voluptuous for mysensors
77028c9 
* Add voluptuous config validation for mysensors
* Remove and clean up parts that are not needed for pymysensors 0.7.
@MartinHjelmare
Copy link
Member Author

Hmm, when I test to config a mysensors platform under the sensor component, I don't get any indication in the log. I can even pass random key-value pairs together with the config for the platform, without any logged errors. This might not be a big deal, in cases where discovery is used, as the platform won't setup unless setup with discovery.

Teagan42
Teagan42 reviewed Aug 27, 2016
View reviewed changes
homeassistant/components/mysensors.py
vol.Required(CONF_GATEWAYS): vol.All(cv.ensure_list, [
{
vol.Required(CONF_DEVICE): cv.string,
vol.Optional(CONF_PERSISTENCE_FILE): cv.string,
Copy link
Contributor

Choose a reason for hiding this comment

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

Persistence_FIle is optionsal, but CONF_PERSISTENCE defaults to true. How does the SerialGateway behave if told to persist without a file?

Copy link
Member Author

Choose a reason for hiding this comment

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

That behavior is still left later in the code. I'm using the index of the gateways to set different default filenames, to not overwrite. Any suggestion how I could solve that in voluptuous?

Copy link
Contributor

Choose a reason for hiding this comment

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

hrm... well we could move persist to the same level as the persistence file - that would allow you to persist some devices, and not others - more granularity

@robbiet480
Copy 8000 link
Member

T-Minus 10 hours until we release 0.27. It would be great if this could make it in time, but no rush if it can't, please don't stress :)

@MartinHjelmare
Copy link
Member Author
MartinHjelmare commented Aug 27, 2016 •
edited
Loading

@robbiet480 Yes, I'll have a look at solving @Teagan42 's question, but everything should be working as it is now, so should be able to get this in before time T.

@MartinHjelmare
Copy link
Member Author

I suggest we merge this as is, and I can look if it's possible to move the persistence file name creation to the config validation, later. I'll leave the decision up to y'all. I'll be at a birthday party the rest of the day, so probably AFK.

@Teagan42 Teagan42 mentioned this pull request Aug 27, 2016
Closed
@Teagan42
Copy link
Contributor

Issue created for persistence file @MartinHjelmare
#3009

@Teagan42 Teagan42 merged commit 6acaf25 into home-assistant:dev Aug 27, 2016
@MartinHjelmare MartinHjelmare deleted the mysensors-voluptuous branch September 16, 2016 16:46
@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
voluptuous
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MartinHjelmare @fabaff @robbiet480 @Teagan42
0