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

Limit update() to only work on primary keys #1862

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

Conversation

coolreader18
Copy link
Collaborator

Description of Changes

@cloutiertyler and I were talking about this. Makes the semantic mapping for people easier, a row update on the client is an update() on the server. Doesn't make sense for non-pk to have .update() but it doesn't trigger an update.

Expected complexity level and risk

1

@gefjon
Copy link
Contributor

gefjon commented Oct 15, 2024

Please also update the API proposal

@gefjon
Copy link
Contributor

gefjon commented Oct 16, 2024

Should we change the API? If updates are only by PK, and a table has at most one PK, then they don't really belong on unique indices, do they? Should it be ctx.db.my_table().update(new_row) rather than ctx.db.my_table.().my_pk_field().update(new_row)?

EDIT: Or do we want to leave it on the PK index to make it clear how the old row is found during the update?

@coolreader18 coolreader18 force-pushed the noa/no-update-for-non-pk branch 2 times, most recently from 4d5a078 to 64a2b2b Compare October 16, 2024 21:22
@coolreader18
Copy link
Collaborator Author

Or do we want to leave it on the PK index to make it clear how the old row is found during the update?

I think that's beneficial - I just added a section in the doc comment for update() about what's considered an update and why it only exists on the primary key. We could probably consider adding .update() on the table in the future, if it's enough of a ergonomics thing.

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