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

feat: Add an upsert_item method #327

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: Add an upsert_item method #327

wants to merge 1 commit into from

Conversation

taion
Copy link
Contributor

@taion taion commented Mar 26, 2020

No description provided.

@taion taion requested a review from itajaja March 26, 2020 15:50
@taion
Copy link
Contributor Author

taion commented Mar 26, 2020

I don't think this is the right way to go. I think we might want more of a update_item_some_more_raw hook that both update_item_raw and create_item_raw call.

item = self.update_item(item, data_in) or item
self.commit()
item = self.upsert_item(item, data_in) or item
updated = inspect(item).persistent
Copy link
Member

Choose a reason for hiding this comment

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

this might not work if for some reason someone changes how the creation and upsert method work. what about upsert_item returns a tuple (item: T, created: bool)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know. Awkward either way. It's why I'd be more inclined to add a new hook.

Copy link
Member

@itajaja itajaja left a comment

Choose a reason for hiding this comment

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

I am fine with this too, it will surely clean up some code. I would change the way you know something is updated by being more explicit as suggested, but up to you. not sure how the hook they you mentioned would look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants