Add a Local To-do component #102627
Add a Local To-do component #102627
Conversation
| async def async_load(self) -> str: | ||
| """Load the calendar from disk.""" | ||
| async with self._lock: | ||
| return await self._hass.async_add_executor_job(self._load) | ||
|
|
There was a problem hiding this comment.
We could use a decorator for this and async_store proxy functions
There was a problem hiding this comment.
Can you elaborate? I'm not familiar with the pattern you are referring to.
I the idea that i'd introduce a new decorator or that there is an existing one that I can use?
There was a problem hiding this comment.
I think we can address this in a follow up, when we know more.
There was a problem hiding this comment.
Sorry for the delay, but somehow I missed your notification.
I have meant to introduce a new decorator for
async with self._lock:
return await self._hass.async_add_executor_job(func)
Where func is either load or save.
The motivation was only the reduce duplicate code but no hurry for this small change :)
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| """Convert a HomeAssistant TodoItem to an ical Todo.""" | ||
| try: | ||
| return Todo.parse_obj(dataclasses.asdict(item, dict_factory=_todo_dict_factory)) | ||
| except ValidationError as err: |
There was a problem hiding this comment.
This looks like the library isn't abstracted properly in this interface. I don't think the library user should know that the library uses pydantic.
There was a problem hiding this comment.
OK -- we'll want to update local calendar as well then. Aside: i chose a different error to raise there given it was about input validation based on different review standards. But here, i'm not sure that the user can actually make this happen given format validation happening upstream on the input so its not expected
Just to be clear though: The way ical uses pydantic is not just an accidental leakage of an internal underlying detail -- the entirely library is built to intentionally make you use data objects that are pydantic data classes (e.g. Event and the examples https://allenporter.github.io/ical/ical.html#quickstart) are directly using the pydantic constructors and helpers as convenience methods rather than having multiple layers of wrapping. (The objects being used here are a BaseModel)
When designing the ical library pydantic didn't seem like it was going to randomly introduce an upgrade nightmare of changes given it was recommended by core team members as a defacto data serialization library, but lesson learned.
The smallest possible change is to add method with the same signature as parse_obj throws an ical specific exception. Larger changes could be changing the base objects to no longer by pydantic objects.
Happy to have any library design advice here given that context.
There was a problem hiding this comment.
I think it's enough to replicate the method and throw an ical specific exception instead of a pydantic exception.
It's not a problem to return BaseModel objects to the user as the user doesn't need to care about the base class. It's just a typed object with attributes as far as the user needs to know.
There was a problem hiding this comment.
I don't see any pydantic specific interfaces or mentions in the linked library docs. That looks good.
There was a problem hiding this comment.
OK, sounds good. Let me fix this in a future library update, if that is alright with you, and i'll update local calendar too.
| break | ||
| if found_item is None: | ||
| raise HomeAssistantError( | ||
| f"Item '{uid}' not found in todo list {self.entity_id}" |
There was a problem hiding this comment.
Is this an api error or a user error?
There was a problem hiding this comment.
Currently this is only called by the websocket so it would indicate the frontend is referencing a stale item (out of sync somehow with core?). If this were exposed as a service, it could mean the user is referencing an object that doesn't exist like racing with a delete, or it could mean a a typo/template error.
Let me know if another exception is more appropriate.
There was a problem hiding this comment.
Let's keep it like this for now. If it would be a service the current convention is to raise ValueError for bad user input.
Co-authored-by: Robert Resch <robert@resch.dev> Co-authored-by: Martin Hjelmare <marhje52@gmail.com>
Remaining comment can be addressed in a follow up
Proposed change
Add a local todo component based on rfc5545. This is similar to shopping list, but supports multiple entities, and stores the local data in ics format. This shares the same underlying library as local calendar that writes .ics files.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc.To help with the load of incoming pull requests: