Add protobuf text format support#13
Conversation
Adds to_text/from_text on Message and a merge_from_text free function, implementing the protobuf text format (.txtpb) for debugging, tests, and config files, as requested in bufbuild#12. Ported from protobuf-es's txtpb implementation and conformance-tested test suite; output matches txtpbfmt's default formatting and passes the full conformance suite. Also fixes is_zero_value to treat -0.0 as non-zero for double fields (previously only excluded for float), which was silently dropping negative zero from JSON/binary output with implicit presence.
|
|
||
| The output matches the default formatting of [txtpbfmt](https://github.com/protocolbuffers/txtpbfmt), so text written by `to_text` can be read by the buf CLI, `protoc`, and other implementations, and text they produce can be read by `from_text`. | ||
|
|
||
| Unlike binary and JSON serialization, `to_text` does not validate legacy required fields: unset fields are simply omitted, so a partially initialized message can always be dumped for debugging. |
There was a problem hiding this comment.
calling this out as a difference for this library, but matching the upstream protobuf-es behavior (IIUC).
There was a problem hiding this comment.
I was thinking about the above diff and this seems to seal the deal that it's probably best to leave text format out from the top-level details, i.e. revert the diff on lines 3-11. I don't want users to think this is a real format that they need to consider alongside the others, it's really a "if you don't know you need it you don't even need to read this section" type of feature. Also of note is that JSON is also "a text format" and arguably much better and we need to make sure that's reflected in these docs
| if isinstance(member, DescFieldValueScalar) and member.scalar in ( | ||
| ScalarType.FLOAT, | ||
| ScalarType.DOUBLE, | ||
| ): | ||
| # Negative zero is not the zero value: it is distinguishable from 0.0 | ||
| # and must survive serialization, matching C++ and protobuf-go. |
There was a problem hiding this comment.
can split this out if you'd rather land it separately.
There was a problem hiding this comment.
Doh, thanks! Can you add a test in test_float.py?
There was a problem hiding this comment.
Hey sorry my issue reply was quite cryptic when reading it again. We should just add a txtpb package with three free functions to contain the less commonly used functionality, even without reexporing I think.
More importantly, we are relatively strict on adding methods to Message since they affect escaping (we would need to add these to escaped keywords). No need to worry about that for text format which can be all free functions.
Per anuraaga's review on bufbuild#13: - Move to_text/from_text/merge_from_text out of Message methods and the top-level protobuf package into a new protobuf.txtpb package of free functions. Message methods are reserved-keyword surface (every new method requires updating _sanitization.py's escape list); free functions avoid that entirely for this less commonly used format. - Drop the colon before a message value's `{` (e.g. `foo {` not `foo: {`), matching the canonical google.protobuf.text_format writer, verified directly against text_format.MessageToString. protobuf-es's writer always includes the colon; this diverges from it deliberately to match the C++/Python reference implementation instead, since the colon is optional in the spec. All internal text-format logic (_to_text.py, _from_text.py) moves into the new protobuf/txtpb/ package as private submodules.
Took a pass at reworking in c70d3c3. |
The new protobuf.txtpb.from_text(MessageType, text) row is wider than the prior column width; re-pad the whole table to keep it readable in raw markdown.
| # protobuf-es's writer always includes it; we deliberately match the | ||
| # canonical C++/Python behavior instead. |
There was a problem hiding this comment.
not sure which behavior we prefer; might be good to confirm that protobuf-es' implementation is deliberately different.
There was a problem hiding this comment.
protobuf-es deliberately sides with protobuf-go where the spec is open (writer.ts). The colon is optional in the spec and all parsers accept both forms, so matching Python's google.protobuf.text_format makes sense here.
|
|
||
| def test_escapes_strings(self) -> None: | ||
| # Controls, quotes, C1 controls, and raw UTF-8. | ||
| msg = Proto3(optional_string='tab\tnl\n"q"\\b\x7f é') |
There was a problem hiding this comment.
this previously had an unprintable control character (from the protobuf-es tests), so moved it to an escape.
anuraaga
left a comment
There was a problem hiding this comment.
Thanks for helping with this!
|
|
||
| The output matches the default formatting of [txtpbfmt](https://github.com/protocolbuffers/txtpbfmt), so text written by `to_text` can be read by the buf CLI, `protoc`, and other implementations, and text they produce can be read by `from_text`. | ||
|
|
||
| Unlike binary and JSON serialization, `to_text` does not validate legacy required fields: unset fields are simply omitted, so a partially initialized message can always be dumped for debugging. |
There was a problem hiding this comment.
I was thinking about the above diff and this seems to seal the deal that it's probably best to leave text format out from the top-level details, i.e. revert the diff on lines 3-11. I don't want users to think this is a real format that they need to consider alongside the others, it's really a "if you don't know you need it you don't even need to read this section" type of feature. Also of note is that JSON is also "a text format" and arguably much better and we need to make sure that's reflected in these docs
|
|
||
| ## Text Format | ||
|
|
||
| The [text format](https://protobuf.dev/reference/protobuf/textformat-spec/) is a plain-text syntax mainly used for debugging, tests, and config files. |
There was a problem hiding this comment.
| The [text format](https://protobuf.dev/reference/protobuf/textformat-spec/) is a plain-text syntax mainly used for debugging, tests, and config files. | |
| In addition to binary, and JSON, Protobuf also provides, [text format](https://protobuf.dev/reference/protobuf/textformat-spec/), a plain-text syntax used for debugging, tests, and config files. |
This follows on https://github.com/bufbuild/protobuf-py/pull/13/changes#r3548113471 and notice removing "mainly" quite intentionally.
BTW what do you think about suggesting JSON instead of text format unless working with an existing system?
There was a problem hiding this comment.
Also how about moving this past Merging to be the last section
| Unlike binary and JSON, text format serialization is exposed as free functions from `protobuf.txtpb`, not as methods on `Message`. | ||
| This keeps the less commonly used text format from adding new reserved attribute names to every generated message class. | ||
|
|
There was a problem hiding this comment.
| Unlike binary and JSON, text format serialization is exposed as free functions from `protobuf.txtpb`, not as methods on `Message`. | |
| This keeps the less commonly used text format from adding new reserved attribute names to every generated message class. |
Careful when Claude repeats prompts as docs ;)
| The output matches the canonical writer used by `google.protobuf.text_format` (the same style `protoc --decode` and other Protobuf implementations produce): two-space indentation, one field per line, and no colon before a message value's `{`. | ||
| Text written by `to_text` can be read by the buf CLI, `protoc`, and other implementations, and text they produce can be read by `from_text`. | ||
|
|
There was a problem hiding this comment.
| The output matches the canonical writer used by `google.protobuf.text_format` (the same style `protoc --decode` and other Protobuf implementations produce): two-space indentation, one field per line, and no colon before a message value's `{`. | |
| Text written by `to_text` can be read by the buf CLI, `protoc`, and other implementations, and text they produce can be read by `from_text`. |
This is true of at least binary too and isn't something we usually doc here I think
| merge_from_json(user, text) | ||
|
|
||
| # Merge text format into an existing message | ||
| merge_from_text(user, text) |
There was a problem hiding this comment.
If moving the text format docs to the last section, I think we can remove these from this one and keep them all together under Text Format
| assert got == want, text | ||
| assert to_text(got) == text | ||
|
|
||
| def test_preserves_a_broad_proto2_message(self) -> None: |
There was a problem hiding this comment.
| def test_preserves_a_broad_proto2_message(self) -> None: | |
| def test_proto2(self) -> None: |
etc. The test name generally just describes the case structure, not the assertion. A docstring could be the more verbose assertion description but most tests shouldn't need one
There was a problem hiding this comment.
took a pass at updating these, but maybe too wide a swath; let me know...
- Rename protobuf.txtpb's to_text/from_text to message_to_text/ message_from_text, matching the message_to_json_value/ message_from_json_value convention; add ignore_unknown_fields to message_from_text/merge_from_text for symmetry with the JSON API. - Convert _Token.kind in the text format tokenizer from string literals to a _TokenKind enum; drop the unneeded sign param from _read_bool_value; fold the to_text writer's empty-message collapse into end_message() instead of external mark/reset calls; import the shared DEPTH_LIMIT instead of a local constant; assorted docstring and comment trims flagged as overly verbose. - Pare back the docs: text format is no longer listed alongside binary/JSON as a peer format, its docs section moved to the end of serialization.md, and redundant explanatory asides were cut from DESIGN.md, README.md, and the txtpb package docstring. - Add the missing negative-zero regression test for double fields requested alongside the is_zero_value fix. - Rename all test_text_format.py test methods to short, case-describing names, per reviewer convention.
|
@anuraaga might have to come back for another round of fixups here, but in the meantime feel free to push directly to this branch if that helps with iteration :). hopefully I (claude) got most things! |
anuraaga
left a comment
There was a problem hiding this comment.
I applied a round of cleanups directly, just have one question left
| if negative: | ||
| msg = f'invalid float value "-{tok.text}"' | ||
| raise ValueError(msg) | ||
| match tok.text.lower(): |
There was a problem hiding this comment.
Ah thanks for checking - I also double checked the spec page and it seems to show infinity etc has literals without case insensitivity. So this line should be match tok.text instead of match tok.text.lower() right? Double check I'm not misunderstanding it
| if negative: | ||
| msg = f'invalid float value "-{tok.text}"' | ||
| raise ValueError(msg) | ||
| match tok.text.lower(): |
There was a problem hiding this comment.
Found the relevant code in protobuf-go and the callout to C++ doing that, so this looks good!
Implements the protobuf text format (
.txtpb) for debugging, tests, and config files, as requested in #12. Ported from protobuf-es's txtpb implementation and conformance-tested test suite; passes the full conformance suite.to_text/from_text/merge_from_textare free functions in a newprotobuf.txtpbpackage, notMessagemethods — this keeps the less commonly used text format from adding new reserved attribute names to every generated message class.Output matches the canonical writer used by
google.protobuf.text_format(verified directly againsttext_format.MessageToString): two-space indentation, and no colon before a message value's{(e.g.foo {notfoo: {). This deliberately diverges from protobuf-es's writer, which always includes the colon, since the colon is optional in the text-format spec and the C++/Python reference implementation omits it.Also fixes
is_zero_valueto treat-0.0as non-zero for double fields (previously only excluded for float), which was silently dropping negative zero from JSON/binary output with implicit presence.Fixes #12.