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

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

Closed
wants to merge 2 commits into from

Conversation

serkangunes
Copy link

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

Coverage Status

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

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

Ok shall I close this pull request then?

@serkangunes
Copy link
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
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
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.

None yet

3 participants