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

[uniffi] Rename commit to key_update #139

Closed
wants to merge 1 commit into from

Conversation

mgeisler
Copy link
Contributor

@mgeisler mgeisler commented Apr 3, 2024

Issues:

Addresses #81.

Description of changes:

When discussing this internally, the feeling was that commit would mostly be used to refresh keys. The name key_update would thus be more descriptive.

Call-outs:

Since the method can also be used to commit received proposals (if/when we support them), the current name might be better?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT license.

When discussing this internally, the feeling was that `commit` would
mostly be used to refresh keys. The name `key_update` would thus be
more descriptive.

Addresses awslabs#81.
@mgeisler mgeisler requested a review from a team as a code owner April 3, 2024 13:43
@mgeisler
Copy link
Contributor Author

mgeisler commented Apr 3, 2024

Since the method can also be used to commit received proposals (if/when we support them), the current name might be better?

The changes in the tests show this a bit, but I'll let people here discuss 😄

@mulmarta
Copy link
Contributor

mulmarta commented Apr 3, 2024

I'm not so sure about this one...

  1. This could have 2 purposes: PCS as key_update indicates or committing received proposals
  2. Depending on your commit_options and received proposals, this may NOT update any keys. E.g. if you received add proposals and your commit_options don't enforce path, then this will end up being a partial commit i.e. it doesn't update any keys

@beurdouche
Copy link

Without having looked into this much, a side note is that we should avoid non standard terminology as much as possible.

@mgeisler
Copy link
Contributor Author

mgeisler commented Apr 3, 2024

Thanks for the feedback! Let me close this for now since it doesn't sound like a good idea.

@mgeisler mgeisler closed this Apr 3, 2024
@mgeisler mgeisler deleted the uniffi-commit-to-keyupdate branch April 11, 2024 13:40
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.

3 participants