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 To-do due date/time and description service calls #29900

Merged
merged 6 commits into from
Nov 29, 2023

Conversation

allenporter
Copy link
Contributor

Proposed change

Add To-do due date/time and description service call documentation.

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.

@allenporter allenporter requested a review from a team as a code owner November 18, 2023 16:26
@home-assistant home-assistant bot added has-parent This PR has a parent PR in a other repo next This PR goes into the next branch labels Nov 18, 2023
source/_integrations/todo.markdown Outdated Show resolved Hide resolved
source/_integrations/todo.markdown Outdated Show resolved Hide resolved
source/_integrations/todo.markdown Outdated Show resolved Hide resolved
source/_integrations/todo.markdown Outdated Show resolved Hide resolved
frenck
frenck previously approved these changes Nov 28, 2023
Copy link
Member

@frenck frenck left a comment

Choose a reason for hiding this comment

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

✅ Approved. Can be merged as soon as the parent PR gets merged.

@frenck frenck added the awaits-parent Awaits the merge of an parent PR label Nov 28, 2023
@@ -61,6 +61,11 @@ Add a new To-do Item. A To-do list `target` is selected with a [Target Selector]
| Service data attribute | Optional | Description | Example |
| ---------------------- | -------- | ----------- | --------|
| `item` | no | the name of the to-do Item. | Submit income tax return
| `due_date` | yes | The date the to-do item is expected to be completed. | 2024-04-10
| `due_date_time` | yes | The date and time the to-do item is expected to be completed. | 2024-04-10 23:00:00
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, now that I read this I'm thinking it should be due_datetime instead. Is there a reason to write it date_time instead of datetime?

Copy link
Member

Choose a reason for hiding this comment

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

Or just due?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but that's more than a name change.

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 rationale for the current name is only to match the convention of https://www.home-assistant.io/integrations/google/#service-googlecreate_event -- I don't have a preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To elaborate -- i totally know what you are talking about here, and was thinking about this when adding it.

"due_datetime" would be the way to name this to make it like a python name.

"due_date" and "due_date_time" seemed to be reasonable from a user perspective so i went with that name given it was that style in calendar.

(What i'd really love is "due" with a combined date or date time selector)

Happy to adjust if there are already conventions or preferences.

Copy link
Member

Choose a reason for hiding this comment

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

Hmz I see your point, just due (combined) won't work because of selectors, so that makes total sense.

I'm personally in the middle if it boils down to date_time vs datetime, although from a consistency perspective:

https://www.home-assistant.io/integrations/datetime/

Copy link
Contributor

Choose a reason for hiding this comment

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

for context: there are currently 11 files in the docs that use date_time, and 107 that mention datetime (over 400 occurrences).

Copy link
Member

Choose a reason for hiding this comment

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

We decided to go ahead with the rename. The google calendar parameter should probably be renamed too at some point, but that'll be a breaking change.

@MartinHjelmare MartinHjelmare added parent-merged The parent PR has been merged already and removed awaits-parent Awaits the merge of an parent PR labels Nov 28, 2023
following review comment by frenck.
@MartinHjelmare
Copy link
Member

I've opened home-assistant/core#104698 to rename the parameter to due_datetime.

source/_integrations/todo.markdown Outdated Show resolved Hide resolved
source/_integrations/todo.markdown Outdated Show resolved Hide resolved
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

The rename of due_date_time to due_datetime for the todo integration is complete. This can be merged. Thanks!

@MartinHjelmare MartinHjelmare merged commit a478880 into home-assistant:next Nov 29, 2023
6 checks passed
@home-assistant home-assistant bot removed the parent-merged The parent PR has been merged already label Nov 29, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Nov 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
has-parent This PR has a parent PR in a other repo next This PR goes into the next branch
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants