Adds port/SSL config options for RainMachine #8986
Conversation
| @@ -42,10 +43,12 @@ | |||
| vol.Email(), # pylint: disable=no-value-for-parameter | |||
| vol.Required(CONF_PASSWORD): | |||
| cv.string, | |||
| vol.Optional(CONF_PORT): | |||
| vol.Optional(CONF_ZONE_RUN_TIME, default=DEFAULT_ZONE_RUN_SECONDS): | ||
| cv.positive_int, | ||
| vol.Optional(CONF_HIDE_DISABLED_ENTITIES, default=True): |
There was a problem hiding this comment.
Is removing this a breaking change?
There was a problem hiding this comment.
Not really:
- The functionality of this parameter (a) was acting poorly with the device and (b) didn't provide any tangible benefit.
- This parameter was disabled and undocumented to begin with.
- Based on the changes made at the bottom of
async_setup_platform, even if a user leaves this option in the config, nothing will break.
There was a problem hiding this comment.
TLDR: Ok.
Even if the change doesn't break a user's set up it's good to mark the PR as breaking if the user looses configurability. In this case, since the option was not documented, and it's a young platform, it could be ok to not mark it as breaking. Although, if there's a feature or non-feature in the code, you can usually be sure that there's someone that depends on it.
There was a problem hiding this comment.
Got it, good to know – will keep that in mind going forward.
Although, if there's a feature or non-feature in the code, you can usually be sure that there's someone that depends on it.
That made me LOL. 😆
| entities.append( | ||
| RainMachineProgram( | ||
| client, program, device_name=rainmachine_device_name)) | ||
| if program.get('active'): |
There was a problem hiding this comment.
I liked the guard clause that was used before. But do as you like here.
There was a problem hiding this comment.
The guard is still there – it just looks different (especially considering my above comment):
Previous: "If a program/zone is disabled and hide_disabled_entities is true, skip it."
New: "If a program/zone is enabled, show it."
There was a problem hiding this comment.
I was referring to using a guard clause to let the normal path of execution be not indented. But the code is not nested here so it doesn't matter really.
There was a problem hiding this comment.
It's your call: I'm happy to accommodate. This flow is more readable to me (only include something if it's enabled), but if you'd prefer a guard clause, I can certainly do it.
| RainMachineProgram( | ||
| client, program, device_name=rainmachine_device_name)) | ||
| if not program.get('active'): | ||
| pass |
|
Appreciate your help, @MartinHjelmare! |
* Adding port/SSL config updates * New requirements generated * Made `port` and `ssl` parameters optional * Add defaults for new parameters * Re-adding guard clause * pass > continue
Description:
Adds the ability to specify two more configuration options when querying a RainMachine locally:
port: the TCP port used by the device for its REST API (default: 8080)ssl: whether HTTPS should be used on all requests (default: true)Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#3180
Example entry for
configuration.yaml(if applicable):Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
REQUIREMENTSvariable (example).requirements_all.txtby runningscript/gen_requirements_all.py.