feat(cli): add avro support #1735
Conversation
| * @param {FormatType} [format] - The format to convert the message to. | ||
| * @returns {Buffer | string} - The processed message. | ||
| */ | ||
| const processPublishMessage = ( | ||
| message: string | Buffer, | ||
| protobufPath?: string, | ||
| protobufMessageName?: string, | ||
| schemaOptions: SchemaOptions, | ||
| format?: FormatType, | ||
| ): Buffer | string => { |
There was a problem hiding this comment.
I believe that schemaOptions should be an optional parameter, and that schemaOptions and format should be combined into a single options object.
The code could be refactored like this:
const processReceivedMessage = (
payload: Buffer,
options?: {
format?: "base64" | "json" | "hex" | "cbor" | "binary"
schema?: 'protobuf' | 'avro'
protobufPath?: string
protobufMessageName?: string
avscPath?: string
},
): string | Buffer => {The rationale is that these parameters are all related to processing the payload and are optional. By merging them into a single options object, we eliminate concerns about argument order and make the code more intuitive and easier to maintain.
There was a problem hiding this comment.
Thank you for your effort on reviewing my code. But I have a different opinion on the design of this parameter.
formatandschemaOptionsserve different purposes within the processing pipeline.(They are used in different functions)- merging them into one parameter would bypass the restriction set in type
SchemaOptions, which I believe is kind of unsafe.
But I do understand your suggestion on simplifying the signature of this function, maybe we can come to a middle ground which retains safety while simplifies the signature.
add JSON validation fix typo & doc comment
|
@LAST7 Thanks for your contribution and everything LGTM. |
|
@LAST7 Thanks for the PR! |
PR Checklist
If you have any questions, you can refer to the Contributing Guide
What is the current behavior?
Support only protobuf as schema-based encoding. (cli)
Issue Number
None
What is the new behavior?
Support avro encoding now. (cli)
Does this PR introduce a breaking change?
Specific Instructions
Other information
I wonder what tests should be written to test the function and stability.