grpc-node icon indicating copy to clipboard operation
grpc-node copied to clipboard

docs(proto-loader): fix docs for `options.bytes`

Open kilianc opened this issue 4 years ago • 7 comments

kilianc avatar Jun 08 '21 11:06 kilianc

CLA Signed

The committers are authorized under a signed CLA.

  • :white_check_mark: Kilian Ciuffolo (3623179c183cb8fc0609bb53c54ba80993f84061)

To clarify, the way this actually works is that any string other than the ones currently listed will cause the default behavior, which is to use Buffer. The same is true of the longs and enums options. I would be OK with listing another option that results in the default behavior, but I'd rather do it for all three of those options if we're going to do it at all, for consistency.

murgatroid99 avatar Jun 08 '21 16:06 murgatroid99

To clarify, the way this actually works is that any string other than the ones currently listed will cause the default behavior, which is to use Buffer. The same is true of the longs and enums options. I would be OK with listing another option that results in the default behavior, but I'd rather do it for all three of those options if we're going to do it at all, for consistency.

Do you mean that an invalid value will trigger the default behavior which is Buffer, even the Buffer string itself? No error is thrown?

kilianc avatar Jun 09 '21 16:06 kilianc

Essentially, yes. I would say that values other than the ones listed are ignored, leaving the default behavior unchanged.

This CLI is directly based on the Protobuf.js API, and in that API, there are values that modify the default behavior (e.g. bytes: String or bytes: Array), and leaving it unset results in the default behavior. That's why there's a default listed that doesn't correspond to one of the listed options.

murgatroid99 avatar Jun 09 '21 18:06 murgatroid99

Essentially, yes. I would say that values other than the ones listed are ignored, leaving the default behavior unchanged.

This CLI is directly based on the Protobuf.js API, and in that API, there are values that modify the default behavior (e.g. bytes: String or bytes: Array), and leaving it unset results in the default behavior. That's why there's a default listed that doesn't correspond to one of the listed options.

I believe I should update the readme some more then

kilianc avatar Jun 10 '21 01:06 kilianc

anything else I should fix in the PR?

kilianc avatar Dec 21 '21 21:12 kilianc

What I don't like here is the inconsistency of specifying an additional default value in the valid values for the bytes option, but not other options. We should either do all or nothing. So, if you want to have this change, you should also do the same for longs and enums.

murgatroid99 avatar Jan 04 '22 17:01 murgatroid99