Format default values for "present" fields regardless of value #7487
Conversation
| // If we know about presence for the field, use that to determine whether | ||
| // or not to write the value. We *don't* use this for (non-oneof) message fields, | ||
| // so that we can control whether null is written or not via FormatDefaultValues. | ||
| if (field.HasPresence && !(field.FieldType == FieldType.Message && field.ContainingOneof == null)) |
There was a problem hiding this comment.
Why can't this just be:
if (field.HasPresence) {
Why exclude messages and oneofs from this? Any field with presence should be skipped if it is not present.
There was a problem hiding this comment.
It's for message fields that aren't in a oneof - so that if you've specified "format default values" you still get null. Otherwise this is a breaking change.
jskeet
force-pushed
the
csharp-conformance
branch
from
0f39431
to
7dc7179
Compare
|
@haberman: I've rewritten this now, in a way that I think is what we discussed. PTAL. We really need to get this specified in a cross-language way in some sort of flow chart... |
|
@haberman: Ping? |
| /// </summary> | ||
| private bool ShouldFormatFieldValue(IMessage message, FieldDescriptor field, object value) | ||
| { | ||
| bool formatDefaultValue = settings.FormatDefaultValues && |
There was a problem hiding this comment.
Can this be simply:
bool formatDefaultValue = settings.FormatDefaultValues && !field.HasPresence
There was a problem hiding this comment.
No. If FormatDefaultValues is set to true, then I'd expect a singular int32 field which is either in a proto2 message or explicitly optional in a proto3 message to be formatted regardless of presence... although I can see arguments in favour of making field presence trump FormatDefaultValues.
I think we should probably decide that point, and then decide the implementation separately.
|
@haberman: I've added an extra commit along the lines you suggest. PTAL, and if you're happy I'll rebase to squash the two commits together (and reword). Note that currently if you set |
Fixes protocolbuffers#7486. Note that this changes the behavior for message fields where "WithFormatDefaultValues(true)" has been specified. This is effectively fixing a bug, but will need to be noted in the release notes. Basically, FormatDefaultValues only affects fields that don't support presence - most commonly, singular primitive non-optional fields in proto3.
jskeet
force-pushed
the
csharp-conformance
branch
from
6dc92ac
to
3e85179
Compare
|
Thanks - have rebased down to one commit (and clarified the FormatDefaultValues documentation). Will merge on green. |
Fixes #7486.
The semantics are relatively complex to explain, even though the
code isn't particularly complex. We need to be certain that these
are the semantics we want.