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

Add services to Renault integration #18981

Merged
merged 4 commits into from
Sep 1, 2021

Conversation

epenet
Copy link
Contributor

@epenet epenet commented Aug 18, 2021

Proposed change

Add services to Renault integration, as a follow up to #14383

Type of change

  • Spelling, grammar or other readability improvements (current branch).
  • Adjusted missing or incorrect information in the current documentation (current branch).
  • Added documentation for a new integration I'm adding to Home Assistant (next branch).
  • Added documentation for a new feature I'm adding to Home Assistant (next branch).
  • Removed stale or deprecated documentation.

Additional information

Checklist

  • This PR uses the correct branch, based on one of the following:
    • I made a change to the existing documentation and used the current branch.
    • I made a change that is related to an upcoming version of Home Assistant and used the next branch.
  • The documentation follows the Home Assistant documentation standards.

@probot-home-assistant probot-home-assistant bot added in-progress This PR/Issue is currently being worked on needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch labels Aug 18, 2021
@probot-home-assistant probot-home-assistant bot added the next This PR goes into the next branch label Aug 18, 2021
@probot-home-assistant
Copy link

It seems that this PR is targeted against an incorrect branch. Documentation updates which apply to our current stable release should target the current branch. Please change the target branch of this PR to current and rebase if needed. If this is documentation for a new feature, please add a link to that PR in your description.
(message by DocsTargetBranch)

@probot-home-assistant probot-home-assistant bot added the has-parent This PR has a parent PR in a other repo label Aug 18, 2021
@epenet epenet marked this pull request as draft August 18, 2021 13:48
@epenet epenet marked this pull request as ready for review August 19, 2021 14:25
@MartinHjelmare MartinHjelmare added parent-merged The parent PR has been merged already and removed in-progress This PR/Issue is currently being worked on needs-rebase The PR has been branched of the wrong base branch or targets an incorrect target branch labels Sep 1, 2021
source/_integrations/renault.markdown Outdated Show resolved Hide resolved
source/_integrations/renault.markdown Outdated Show resolved Hide resolved
- `schedules` can be in the form `{'id':1,...}` when updating a single schedules, or in the form `[{'id':1,...},{'id':2,...},...]` when updating multiple schedules within the same call
- the `id` is compulsory on each `schedule` (should be 1 to 5 depending on the vehicle)
- the `activated` flag is an optional boolean. If it is not provided, then the existing flag will be kept as is.
- the `monday` to `sunday` elements are optional. If they are not provided, then the existing settings will be kept for each day. If they are provided as None, then the existing setting will be cleared. If a value is provided, it must conform to this format `{'startTime':'T12:00Z','duration':15}` where start time is in UTC format and the duration is in minutes.
Copy link
Member

Choose a reason for hiding this comment

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

I noticed now that we only validate the time string as string in the service schema. Could we use the time validator to help users use the correct time string?

https://github.com/home-assistant/core/blob/33fb080c1e7dc5db8e24cc142fd9b1972ef337f3/homeassistant/helpers/config_validation.py#L351-L364

Then we can format the final string from the returned datetime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The CLI version of RenaultAPI has some validation/conversion built in: https://github.com/hacf-fr/renault-api/blob/7c2336c52ed0298a3ed2b8e7dbb77b839676dced/src/renault_api/cli/charge/schedule.py#L202

There is also a draft PR to add validation to the non-CLI version: hacf-fr/renault-api#157

There are some extra rules like the minutes of startTime needs to be in (00,15,30,45) and the total duration needs to be a multiple of 15 minutes (eg. 240 is valid but not 245)
There are also some overlap rules (Monday 23:45 for 120 minutes and Tuesday 00:15 for 30 minutes is invalid)

I'll have a go at a follow-up PR.

Copy link
Contributor Author

@epenet epenet Sep 1, 2021

Choose a reason for hiding this comment

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

I'm not sure when I'll get a chance to work on the new PR (there is also a request from @frenck to implement re-auth).
Should this documentation PR be merged anyway in the meantime?

Copy link
Member

Choose a reason for hiding this comment

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

Sure! The docs are fine.

MartinHjelmare
MartinHjelmare previously approved these changes Sep 1, 2021
| Service data attribute | Required | Description | Example |
| ---------------------- | -------- | ----------- | ------- |
| `vehicle`| yes | device_id of the vehicle |
| `schedules` | yes | Schedule details. Can be a single schedule or a list of schedules | `[{'id':1,'activated':true,'monday':{'startTime':'T12:00Z','duration':15}},<br>{'id':2,'activated':false,'monday':{'startTime':'T12:00Z','duration':240}}<br>]` |
Copy link
Member

Choose a reason for hiding this comment

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

I noticed in the preview that the br tag doesn't work.

https://deploy-preview-18981--home-assistant-docs.netlify.app/integrations/renault/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to generate a preview, but I've moved the example out of the grid to gain some space.

Copy link
Member

Choose a reason for hiding this comment

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

The preview is generated automatically by the CI build. It's linked below among the PR checks.

@klaasnicolaas klaasnicolaas added the new-feature This PR adds documentation for a new Home Assistant feature to an existing integration label Sep 1, 2021
@MartinHjelmare MartinHjelmare merged commit d206045 into home-assistant:next Sep 1, 2021
@probot-home-assistant probot-home-assistant bot removed the parent-merged The parent PR has been merged already label Sep 1, 2021
@epenet epenet deleted the renault-services branch September 1, 2021 20:34
@epenet
Copy link
Contributor Author

epenet commented Sep 1, 2021

Thanks

@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed has-parent This PR has a parent PR in a other repo new-feature This PR adds documentation for a new Home Assistant feature to an existing integration next This PR goes into the next branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants