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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kilianc
Copy link

@kilianc kilianc commented Jun 8, 2021

No description provided.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 8, 2021

CLA Signed

The committers are authorized under a signed CLA.

@murgatroid99
Copy link
Member

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.

@kilianc
Copy link
Author

kilianc commented Jun 9, 2021

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?

@murgatroid99
Copy link
Member

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.

@kilianc
Copy link
Author

kilianc commented Jun 10, 2021

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
Copy link
Author

kilianc commented Dec 21, 2021

anything else I should fix in the PR?

@murgatroid99
Copy link
Member

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.

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.

2 participants