Skip to content
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

fix: proto3 optional scalars should default to null in reflection API #1693

Merged
merged 1 commit into from
Jul 8, 2022

Conversation

dae
Copy link
Contributor

@dae dae commented Feb 27, 2022

#1584 made proto3 optional
scalars default to null when using static/static-module, but the old
behaviour remained when using reflection (eg json-module). This PR
attempts to make the latter case behave the same way (would love to use
static code generation, but the resulting file sizes are too large).

This passes the existing tests, but I do not know the codebase well, so I
am not sure if this is the best approach or not.

protobufjs#1584 made proto3 optional
scalars default to null when using static/static-module, but the old
behaviour remained when using reflection (eg json-module).
dae added a commit to ankitects/anki that referenced this pull request Feb 27, 2022
Protobuf 3.15 introduced support for marking scalar fields like
uint32 as optional, and all of our tooling appears to support it
now. This allows us to use simple optional/null checks in our Rust/
TypeScript code, without having to resort to an inner message.

I had to apply a minor patch to protobufjs to get this working with
the json-module output; this has also been submitted upstream:
protobufjs/protobuf.js#1693

I've modified CardStatsResponse as an example of the new syntax.

One thing to note: while the Rust and TypeScript bindings use optional/
null fields, as that is the norm in those languages, Google's Python
bindings are not very Pythonic. Referencing an optional field that is
missing will yield the default value, and a separate HasField() call
is required, eg:

```
>>> from anki.stats_pb2 import CardStatsResponse as R
... msg = R.FromString(b"")
... print(msg.first_review)
... print(msg.HasField("first_review"))
0
False
```
@alexander-fenster alexander-fenster merged commit d9144de into protobufjs:master Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants