-
Notifications
You must be signed in to change notification settings - Fork 15.6k
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
Add an option to preserve proto names in JsonFormatter #6307
Add an option to preserve proto names in JsonFormatter #6307
Conversation
This has been sitting here for quite a bit. Any updates? @anandolee |
I was looking for this option as well. Any updates on merging this PR? |
Could you also review this as well @jtattermusch? |
@anandolee Any updates on this PR? |
@jskeet @amanda-tarafa this probably deserves to be looked at by a pair of C# expert's eyes - just to make sure nothing was missed (I'll try to take a look as well but can't guarantee). |
} | ||
} | ||
} | ||
#region Copyright notice and license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what has changes about this file that it shows up in the diff?
Unless I missed something, the entire file showing up as modified makes it impossible to see any actual changes to the file (which makes reviewing it very difficult).
Let's avoid "false diffs" like this in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's whitespace-only - quite possibly line endings. I'd really like to see the whitespace-only changes here reverted before we merge; it makes it much hard to trace changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done a quick review as well. It looks good.
But I'm wondering about the usefulness of this without the counterpart change in JsonParser. But there might be something I'm missing here.
} | ||
|
||
[Test] | ||
public void WriteValue_Message_PreserveNames() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only test that has been added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we don't need an option for this in the parser as we already handle using either the JSON field name or the original name (see
private static ReadOnlyDictionary<string, FieldDescriptor> CreateJsonFieldMap(IList<FieldDescriptor> fields) |
However, I'm concerned with per-language feature creep. #6299 observes that the option does exist in Java, but I don't know about any other languages. I think the Protobuf team should decide whether they're happy for this to be a feature in general. If it's approved in general, then this implementation looks broadly okay.
} | ||
} | ||
} | ||
#region Copyright notice and license |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's whitespace-only - quite possibly line endings. I'd really like to see the whitespace-only changes here reverted before we merge; it makes it much hard to trace changes.
@@ -233,7 +233,14 @@ private bool WriteMessageFields(TextWriter writer, IMessage message, bool assume | |||
writer.Write(PropertySeparator); | |||
} | |||
|
|||
WriteString(writer, accessor.Descriptor.JsonName); | |||
if (settings.PreserveProtoFieldNames) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd do this with a conditional operator instead:
WriteString(writer, settings.PreserveProtoFieldNames ? accessor.Descriptor.Name : accessor.Descriptor.JsonName);
/// Whether to use the original proto field names as defined in the .proto file. Defaults to false. | ||
/// </summary> | ||
public bool PreserveProtoFieldNames { get; } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: please remove redundant blank line.
/// <summary> | ||
/// Creates a new <see cref="Settings"/> object with the specified field name formatting option and the current settings. | ||
/// </summary> | ||
/// <param name="preserveProtoFieldNames"><c>true</c> to preserve proto field names; <c>false</c> to convert them to lowerCamelCase.</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I thought we allowed users to specify a different JSON field name to the one that would be inferred by the compiler otherwise. Not sure - but arguably using "false to use the default JSON field name representation" would cover that either way.
This reverts commit 013e115.
Eek - I hadn't seen that this had been merged as-is. |
Fixes #6299