Skip to content

Fixing the packed=true by default problem for proto2 files.#683

Closed
serkangunes wants to merge 2 commits intoprotobufjs:masterfrom
serkangunes:master
Closed

Fixing the packed=true by default problem for proto2 files.#683
serkangunes wants to merge 2 commits intoprotobufjs:masterfrom
serkangunes:master

Conversation

@serkangunes
Copy link
Copy Markdown

Fixing the packed=true by default problem for proto2 files. Packed should be false by default for repeated enums when the syntax is set to proto2.

This check is wrong as the types.packed fields includes only the built in primitive types. But at this point type of the repeated enum is a custom type like test.TestMessage therefore types.packed[type] !== undefined always returns false and code never enters that if condition and it does not set the packed flag to false. However when the syntax is set to proto2 this flag should be always set to false.
Added support for turning on the packed option manually.
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage decreased (-0.04%) to 99.957% when pulling dba295a on serkangunes:master into 2ddb76b on dcodeIO:master.

@dcodeIO
Copy link
Copy Markdown
Member

dcodeIO commented Feb 23, 2017

Manually merged your changes and also added some code to Field#resolve removing unnecessary packed options from parsing (again).

@serkangunes
Copy link
Copy Markdown
Author

Ok shall I close this pull request then?

@serkangunes
Copy link
Copy Markdown
Author

serkangunes commented Feb 23, 2017

I have just had a look at your fix. It does fix my issue however with syntax=proto2 you can still set the repeated fields to be packed. It is just by default unpacked.

var packed = field.getOption('packed');
// There reason packed === undefined check is here because if the user has set the packed option 
// value we should not override it as false.
if (field.repeated && packed === undefined && !isProto3)

@dcodeIO
Copy link
Copy Markdown
Member

dcodeIO commented Feb 23, 2017

field.setOption("packed", false, /* ifNotSet */ true); only sets the value if it is undefined (see).

Is that what you are referring to?

@serkangunes
Copy link
Copy Markdown
Author

Ah good one I didn't notice that parameter. I'll close this.

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