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
base: master
from

Conversation

Projects
None yet
3 participants
@serkangunes

serkangunes commented Feb 20, 2017

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.

serkangunes added some commits Feb 20, 2017

Update parse.js
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.
Update parse.js
Added support for turning on the packed option manually.
@coveralls

This comment has been minimized.

coveralls commented Feb 20, 2017

Coverage Status

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

@dcodeIO

This comment has been minimized.

Owner

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

This comment has been minimized.

serkangunes commented Feb 23, 2017

Ok shall I close this pull request then?

@serkangunes

This comment has been minimized.

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

This comment has been minimized.

Owner

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

This comment has been minimized.

serkangunes commented Feb 24, 2017

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