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

Add all commands to proto #2389

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Yury-Fridlyand
Copy link
Collaborator

No description provided.

@Yury-Fridlyand Yury-Fridlyand added the protobuf Protobuf, proto and protoc label Oct 3, 2024
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner October 3, 2024 23:53
@@ -21,7 +21,7 @@ def test_encode_decode_delimited(self):
b_arr = bytearray()
ProtobufCodec.encode_delimited(b_arr, request)
msg_len_varint = int(b_arr[0])
assert msg_len_varint == 18
assert msg_len_varint == 19
Copy link
Collaborator

Choose a reason for hiding this comment

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

Small question:
What is the context around this change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Very good question actually. Get/set commands used in this test got bigger numbers and now require more bytes to encode that.

Watch = 1605,

//// JSON commands
JsonArrAppend = 2001,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should split this PR into two:

  1. clean up all the existing commands
  2. add the JSON and VSS commands


//// JSON commands

JsonArrAppend = 2001;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should split this PR into two:

  1. clean up all the existing commands
  2. add the JSON and VSS commands

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what do you mean in cleanup?

Copy link
Collaborator

Choose a reason for hiding this comment

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

refactor? move the enums? Cleanup?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just copy-pasted Very Big Enum into multiple places and fixed two broken refs (they were named incorrectly).

@shohamazon
Copy link
Collaborator

my honest question is just why 😐

@Yury-Fridlyand
Copy link
Collaborator Author

my honest question is just why 😐

We won't have merge conflicts in every PR if we add this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protobuf Protobuf, proto and protoc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants