Enhance db.OnConflictOpts #346#456
Conversation
There was a problem hiding this comment.
Pull request overview
This PR improves the ergonomics of the database write/delete APIs by allowing on_conflict to be passed as a case-insensitive string ("error", "overwrite", "skip") while preserving support for the existing OnConflictOpts enum.
Changes:
- Replaced the previous dynamically-created
OnConflictOptswith astr-backed enum and added_normalize_on_conflict()to accept/validate string inputs. - Normalized
on_conflictat the start of multipleDatabase.write_*/delete_*methods to enable case-insensitive string options. - Added tests covering enum usage and string usage (including mixed-case strings) plus invalid string handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/openlifu/db/database.py |
Introduces a string-backed OnConflictOpts and normalizes/validates on_conflict across write/delete entry points. |
tests/test_database.py |
Adds coverage asserting string on_conflict values (case-insensitive) behave equivalently to the enum and invalid strings are rejected. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def write_gridweights(self, transducer_id: str, grid_hash: str, grid_weights, on_conflict: OnConflictOpts = OnConflictOpts.ERROR): | ||
| on_conflict = _normalize_on_conflict(on_conflict) |
There was a problem hiding this comment.
@samueljwu agreed with this: updating the signature needed for valid type annotations in the case that someone wants to pass a str. Even though OnConflictOpts now inherits from str, that doesn't address the type annotation: it allows you to pass an OnConflictOpts where a str is expected, but not vice versa.
ebrahimebrahim
left a comment
ebrahimebrahim
left a comment
There was a problem hiding this comment.
Thanks for this, it was bugging me too!
It looks good to me, I would say just address the copilot comment regarding the type annotations and this should be good to go
|
Makes sense! I've updated the type annotations. Thanks |
| def delete_user(self, user_id: str, on_conflict: OnConflictOpts | str = OnConflictOpts.ERROR) -> None: | ||
| on_conflict = _normalize_on_conflict(on_conflict) | ||
| # Check if the user ID already exists in the database |
| def delete_protocol(self, protocol_id: str, on_conflict: OnConflictOpts | str = OnConflictOpts.ERROR): | ||
| on_conflict = _normalize_on_conflict(on_conflict) | ||
| # Check if the sonication protocol ID already exists in the database |
| on_conflict: Behavior when session doesn't exist ('error' or 'skip') | ||
| """ | ||
| on_conflict = _normalize_on_conflict(on_conflict) |
| def test_on_conflict_accepts_enum_and_strings(example_database: Database): | ||
| assert OnConflictOpts.OVERWRITE.value == "overwrite" | ||
|
|
||
| protocol = Protocol(name="bleh", id="a_protocol_with_string_conflict_option") | ||
| example_database.write_protocol(protocol) | ||
|
|
||
| with pytest.raises(ValueError, match="already exists"): | ||
| example_database.write_protocol(protocol, on_conflict="ERROR") |
|
(Most of the copilot comments are not issues newly introdueced by this PR. It looks good) |
Summary
db.write_*/ delete methods to accept stringon_conflictvalues:"error","overwrite", and"skip", case-insensitiveOnConflictOptsFixes #346
Testing
All existing tests pass
Pre-commit hooks pass