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.

Sign up for GitHub

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

Jump to bottom

add support GetMultipleKeys #148

Merged
merged 1 commit into from
Feb 10, 2025
Merged

add support GetMultipleKeys #148

merged 1 commit into from
Feb 10, 2025

Conversation

pfi79
Copy link
Contributor

@pfi79 pfi79 commented Feb 6, 2025 •
edited
Loading

@pfi79 pfi79 requested a review from a team as a code owner February 6, 2025 17:18
@pfi79 pfi79 mentioned this pull request Feb 6, 2025
Merged
@pfi79 pfi79 force-pushed the get-multiple-keys branch from 1ed2041 to f5d3f0a Compare February 6, 2025 17:37
denyeart
denyeart reviewed Feb 6, 2025
View reviewed changes
@pfi79 pfi79 force-pushed the get-multiple-keys branch 2 times, most recently from 2ed9084 to 587d79d Compare February 7, 2025 04:56
bestbeforetoday
bestbeforetoday reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday 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 not very keen on the naming in the public API. I wonder if either GetStates() or GetMultipleStates() would be more straightforward?

From a user perspective, perhaps a varargs keys ...string parameter would be easier to use than requiring them to construct a slice. What do you think? Do we expect the application code to typically already have the keys they want in a slice, or are we forcing them to create a slice unnecessarily by having a slice parameter?

@pfi79
Copy link
Contributor Author

pfi79 commented Feb 7, 2025

I am not very keen on the naming in the public API. I wonder if either GetStates() or GetMultipleStates() would be more straightforward?

From a user perspective, perhaps a varargs keys ...string parameter would be easier to use than requiring them to construct a slice. What do you think? Do we expect the application code to typically already have the keys they want in a slice, or are we forcing them to create a slice unnecessarily by having a slice parameter?

Didn't think about it.

But if you think it is better to rename it to GetMultipleStates and GetPrivateDataMultiple. Easy.

Rearranging to keys ...string is also possible. It still checks inside that there must be at least one key.

bestbeforetoday
bestbeforetoday reviewed Feb 7, 2025
View reviewed changes
@pfi79 pfi79 force-pushed the get-multiple-keys branch from 587d79d to 7119369 Compare February 7, 2025 10:53
@pfi79
Copy link
Contributor Author

pfi79 commented Feb 7, 2025

I am not very keen on the naming in the public API. I wonder if either GetStates() or GetMultipleStates() would be more straightforward?

From a user perspective, perhaps a varargs keys ...string parameter would be easier to use than requiring them to construct a slice. What do you think? Do we expect the application code to typically already have the keys they want in a slice, or are we forcing them to create a slice unnecessarily by having a slice parameter?

done

bestbeforetoday
bestbeforetoday reviewed Feb 7, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

Is there supposed to be a "multiple" variant of GetPrivateDataHash too?

@pfi79 pfi79 force-pushed the get-multiple-keys branch 2 times, most recently from 4f1f35b to 376b5b0 Compare February 7, 2025 12:30
@pfi79
Copy link
Contributor Author

pfi79 commented Feb 8, 2025

@bestbeforetoday There are or will be more comments?

bestbeforetoday
bestbeforetoday reviewed Feb 8, 2025
View reviewed changes
@pfi79 pfi79 force-pushed the get-multiple-keys branch 4 times, most recently from b100d49 to 7114542 Compare February 8, 2025 19:48
C0rWin
C0rWin reviewed Feb 9, 2025
View reviewed changes
@pfi79 pfi79 force-pushed the get-multiple-keys branch from 7114542 to fb8b2bd Compare February 9, 2025 15:03
@pfi79
Copy link
Contributor Author

pfi79 commented Feb 9, 2025

@denyeart I think we're ready to finalise these changes.

bestbeforetoday
bestbeforetoday reviewed Feb 9, 2025
View reviewed changes
@pfi79
add support GetMultipleKeys
b8341ae 
Signed-off-by: Fedor Partanskiy <[email protected]>
@pfi79 pfi79 force-pushed the get-multiple-keys branch from fb8b2bd to b8341ae Compare February 10, 2025 04:58
bestbeforetoday
bestbeforetoday approved these changes Feb 10, 2025
View reviewed changes
Copy link
Member

@bestbeforetoday bestbeforetoday left a comment

Choose a reason for hiding this comment

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

LGTM

@C0rWin C0rWin merged commit 4a4548a into hyperledger:main Feb 10, 2025
6 checks passed
@pfi79 pfi79 deleted the get-multiple-keys branch February 10, 2025 11:23
@pfi79
Copy link
Contributor Author

pfi79 commented Feb 10, 2025

@denyeart please make a release

@denyeart
Copy link
Contributor

@pfi79 released - https://github.com/hyperledger/fabric-chaincode-go/releases/tag/v2.2.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denyeart denyeart denyeart left review comments

@C0rWin C0rWin C0rWin approved these changes

@bestbeforetoday bestbeforetoday bestbeforetoday approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pfi79 @denyeart @C0rWin @bestbeforetoday