Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 a Local To-do component #102627
Add a Local To-do component #102627
Changes from all commits
6cb2148
68b00f8
0163b3a
610a783
a9a7b41
87943a9
b4ba165
e8d839a
328ecdd
5ff6324
ccc160c
6d2790a
79299b9
b979230
166f055
bee1f60
59adec4
a1f2425
2029017
cab3577
431f633
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a decorator for this and
async_store
proxy functionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can address this in a follow up, when we know more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay, but somehow I missed your notification.
I have meant to introduce a new decorator for
Where
func
is either load or save.The motivation was only the reduce duplicate code but no hurry for this small change :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aBaseModel
)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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an api error or a user error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.