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

expose op.isOptsWithFromKey and op.isOptsWithPrefix #16224

Conversation

CaojiamingAlan
Copy link
Contributor

@ahrtr
Copy link
Member

ahrtr commented Jul 14, 2023

They are already supported in #13334.

@CaojiamingAlan
Copy link
Contributor Author

They are already supported in #13334.

The exported methods in #13334 takes a list of OpOption as parameter, which means the user needs to keep the options of the op if he or she wants to check the fields isOptsWithFromKey and isOptsWithPrefix later. what I am trying to do here is to expose those fields directly.

@ahrtr
Copy link
Member

ahrtr commented Jul 14, 2023

if he or she wants to check the fields isOptsWithFromKey and isOptsWithPrefix later

Yes, I understand that, but actually I don't see a real such use case. After the WithFromKey or WithPrefix is evaluated, users only care about the field end.

Of course, adding two methods (what this PR does) is harmless, but also unnecessary. I am neutral to this.

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

This gets a tentative approval from me. At the code level I see no issue, and we have had #16217 opened by a user asking for this for the purpose of making detailed logs in an application.

It still seems like a nice to have more than anything so I defer to maintainers to make final decision. @changhoi please feel free to add any additional context here or in the issue in support of the pr.

@changhoi
Copy link

As described in PR, if the feature is added, our team will add more details to the log depending on whether Operation has a Prefix option.

Also, since there are other flag accessors, it seems natural to add them if there are no problems :)

@ahrtr ahrtr merged commit ff411f5 into etcd-io:main Jul 15, 2023
@CaojiamingAlan CaojiamingAlan deleted the expose_isOptsWithFromKey_and_isOptsWithPrefix branch July 17, 2023 18:47
@CaojiamingAlan CaojiamingAlan restored the expose_isOptsWithFromKey_and_isOptsWithPrefix branch July 17, 2023 18:47
@CaojiamingAlan CaojiamingAlan deleted the expose_isOptsWithFromKey_and_isOptsWithPrefix branch July 17, 2023 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Op.isOptsWithFromKey & Op.isOptsWithPrefix accessor
4 participants