Create Protobuf.Text, create Protobuf.field_presence/2
#398
Conversation
|
Thank you, @whatyouhide. I'll explore using |
v0idpwn
force-pushed
the
feat/text-encoding
branch
from
dcc90cb to
6c73550
Compare
|
Pushed a new version using Some limitations/changes:
I'll address the other comments over the week :) |
|
@v0idpwn OK sounds good, lmk when this is gonna be ready for re-review. 🙃 |
|
I think it's okay to be reviewed, @whatyouhide :) |
|
Actually, I found that there are conformance tests for text encoding. I'll work on them. |
v0idpwn
force-pushed
the
feat/text-encoding
branch
from
804a150 to
006ee2f
Compare
|
@v0idpwn is this ready for review? |
* Allows encoding to textproto
* Run conformance for text, fixes for conformance, add unknown fields support
Most of the conformance tests in the text suite are for decoding, and
some of them are for unknown fields encoding rules, so we only got value
out of five of them. Still, they led to significant improvements in the
implementation.
For one, I figured that the "outer message" **can't** use brackets ("{",
"}"). I thought they were optional and just not used in the examples. So
we went from:
```elixir
Protobuf.Text.encode(%MyMessage{a: 1, b: 2})
```
To:
```elixir
Protobuf.Text.encode(%MyMessage{a: 1, b: 2})
```
I also added unknown fields support, but wasn't able to pass all the
unknown fields tests, so I skipped them for now. The current version
does the job of printing them reasonably well, so I'm not too bothered
with it.
* Create Protobuf.Presence module Unifies presence tracking between encoders. Provides a public API for checking field presence * Add test for messages, fix proto3 * Improve docs
Also fix examples, and add an extra example about repeated
v0idpwn
force-pushed
the
feat/text-encoding
branch
from
8c6bd63 to
7560e91
Compare
| a: 1 | ||
| e: { | ||
| a: 1, |
There was a problem hiding this comment.
I'm not very happy with this inconsistency: notice that the "outside" entries don't have ,, but the nested ones do. Tried to make it match but failed.
This is fine per the spec, I think we can leave it as it is, at least for now.
v0idpwn
changed the title
Protobuf.field_presence/2
|
@whatyouhide I think it is. I did a rebase, removed stuff that was already merged (mostly related to CI), and tried to leave commits more optimized for a non-squash merge given the large scope of this PR (sorry about that, won't do it again 💀). Overall, I’m not really happy with the code, but I’d rather do further iterations in separate, smaller PRs (the interface can be considered stable). I did the google bump here because I wanted to ensure we had the latest conformance tests for text as well. I can move it to a separate PR if you wish. |
Maps should be considered present when used for messages
|
@v0idpwn can we split up |
|
Will do. Will split the bump as well. |
This PR introduces support for encoding Protobuf messages into the textproto format.
Motivation
The primary motivation for this feature is to improve the ability to inspect and visualize Protobuf messages, while still conforming to some standard. The inspect protocol default implementation can be painful to work with large or complex structures. Finding the relevant, non-default fields in a inspected struct is not intuitive and takes an unnecessary amount of time. A textproto representation provides a human-readable alternative that is much closer to the actual data transmitted over the wire (specially when using transformers).
This implementation
and tab size areis configurable.Google.Protobuf.Any.Future Considerations
I'm open to extending this implementation in the future to include:
Google.Protobuf.Any.