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: rename cache hit string value field #272

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

pgautier404
Copy link
Contributor

I tried to achieve clarity and consistency with these names, but I am open to alternatives if these don't seem right to folks.

@malandis
Copy link
Contributor

malandis commented Oct 5, 2022

We should align (across the SDKs) if the mechanism to get the payload a string should be a method or not.

cprice404
cprice404 previously approved these changes Oct 5, 2022
Copy link
Contributor

@cprice404 cprice404 left a comment

Choose a reason for hiding this comment

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

I'm okay with these names but it'd probably be good to get another vote or two for consensus.

If I was going to propose any alternative names they would be something like ValueBytes and ValueString, or ValueAsBytes or ValueAsByteArray and ValueAsString or something. The key difference being that if both names begin with Value then they will be grouped together in the code completion in the IDE, which I think might be... ahem... valuable. :) But I don't feel incredibly strongly about it.

If we want to align across SDKs on whether these should be "properties" or methods (as per michael's comment), then my vote is to make them methods in all cases. It's the most language-proof thing to do and these are actually both methods anyway.

none of these are blockers, just food for thought as you drive toward consensus on it.

@pgautier404
Copy link
Contributor Author

If I was going to propose any alternative names they would be something like ValueBytes and ValueString, or ValueAsBytes or ValueAsByteArray and ValueAsString or something. The key difference being that if both names begin with Value then they will be grouped together in the code completion in the IDE, which I think might be... ahem... valuable. :) But I don't feel incredibly strongly about it.

That is a really good point. I'd be happy to switch to having Value as a prefix. @malandis @poppoerika do you have opinions or preferences on the naming?

If we want to align across SDKs on whether these should be "properties" or methods (as per michael's comment), then my vote is to make them methods in all cases. It's the most language-proof thing to do and these are actually both methods anyway.

I really don't have a preference either way as long as they're consistent. I'll go ahead and switch them to methods.

@malandis
Copy link
Contributor

malandis commented Oct 5, 2022

If I was going to propose any alternative names they would be something like ValueBytes and ValueString, or ValueAsBytes or ValueAsByteArray and ValueAsString or something. The key difference being that if both names begin with Value then they will be grouped together in the code completion in the IDE, which I think might be... ahem... valuable. :) But I don't feel incredibly strongly about it.

That is a really good point. I'd be happy to switch to having Value as a prefix. @malandis @poppoerika do you have opinions or preferences on the naming?

If we want to align across SDKs on whether these should be "properties" or methods (as per michael's comment), then my vote is to make them methods in all cases. It's the most language-proof thing to do and these are actually both methods anyway.

I really don't have a preference either way as long as they're consistent. I'll go ahead and switch them to methods.

I'm not as sensitive to the naming -- just keep in mind we will have to make the data structures consistent too. So BytesBytesDictionary (that's what it is now) would have to be changed to DictionaryBytesBytes

I do want fields that are just properties to be attributes/properties as opposed to methods, where it is language appropriate.

@pgautier404 pgautier404 merged commit d4e4b6e into main Oct 6, 2022
@pgautier404 pgautier404 deleted the rename-cache-get-string-field branch October 6, 2022 19:54
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