-
Notifications
You must be signed in to change notification settings - Fork 101
feat: Implement note-related methods #1084
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
Conversation
0acc7c6 to
81d9ce6
Compare
Coverage Report for backend
|
81d9ce6 to
1fe38e2
Compare
thomass-dev
left a comment
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.
Thanks for your work.
There is 2 important comments, about the user API and the type checking in <set|get|delete>_notes.
The other comments are about documentation and typing, which can be applied to all item files.
|
@MarieS-WiMLDS fyi, this PR can be considered as a breaking-change because it will intend to get a new field in our persistence. When calling |
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 know why my last request changes is not taken into account by GH)
For coherence with the rest of the codebase
cbd9291 to
8da0ba8
Compare
New methods have been added to `Project`: ```python # The default is to act on the latest version project.set_note(key, message, version=-1) project.get_note(key, message, version=-1) project.delete_note(key, message, version=-1) ``` To this end, - `Item` now has an additional attribute, `note`, which is a `str | None` - `ItemRepository` has similar methods The note related methods fail with a KeyError when the key-version combination does not exist, but: - `get_note` returns `None` if the note is `None` (no error) - `delete_note` never errors if the key-version combination exists (even if the note is already `None`, it will be set to `None` again) `set_note` fails with a TypeError if `key` or `message` is not a string. Addresses part of probabl-ai#1041
New methods have been added to
Project:To this end,
Itemnow has an additional attribute,note, which is astr | NoneItemRepositoryhas similar methodsThe note related methods fail with a KeyError when the key-version combination does not exist, but:
get_notereturnsNoneif the note isNone(no error)delete_notenever errors if the key-version combination exists (even if the note is alreadyNone, it will be set toNoneagain)set_notefails with a TypeError ifkeyormessageis not a string.Addresses part of #1041