tafia / quick-protobuf

A rust implementation of protobuf parser
MIT License
446 stars 82 forks source link

Add field presence #243

Closed snproj closed 1 year ago

snproj commented 1 year ago

Changes discussed in #235.

@lexxvir @nerdrew @thomaseizinger feel free to have a look see!

Fishrock123 commented 1 year ago

As discussed in https://github.com/tafia/quick-protobuf/issues/235#issuecomment-1372718617 this approach (Option directly) seems suitable for this library.

This PR coves my use case and solves my problems (at least any that I currently know about).

snproj commented 1 year ago

oops misclick on the review request 😅 sorry nerdrew if you got any notification spam

my plan is to merge this in a few days to a week if there's no pushback; a review before then would be more ideal but I'll try and merge it regardless before too long so I can merge other things

thomaseizinger commented 1 year ago

@snproj Are you still working on this? We currently migrating to quick-protobuf but I think we need this branch before we can roll that out to handle proto3 correctly.

thomaseizinger commented 1 year ago

Also, see https://github.com/MarcoPolo/proto3-and-2-compat-experiments/pull/1/commits/a471f82570b1e1d333e770ac37072d93ba91e540#diff-eb8fa6a5f4a35278dc63958d6e54c1a1674b4d34b513a91a14bb9ae2a51cad35 for a real-world application of this branch. Like I commented above, it would be nice to keep that diff to a minimum (unless you feel strongly about the stylistic changes).

thomaseizinger commented 1 year ago

Friendly ping @snproj . Let me know if there is anything I can help with to push this over the finish line, we are quite interested in having these changes land!

snproj commented 1 year ago

Hello! So, so sorry, stuff cropped up on my end and I took a bit of a long break 😅 I'm back now, and will generally go through everything I've missed out over the past weeks.

thomaseizinger commented 1 year ago

Hello! So, so sorry, stuff cropped up on my end and I took a bit of a long break 😅 I'm back now, and will generally go through everything I've missed out over the past weeks.

No worries at all! Like I said, happy to take over any work here as well if you want! We are interested in having this land in for rust-libp2p.

snproj commented 1 year ago

Just a quick update, I've made most of the changes above; the main thing left to do is the getters and setters, because codegen is a bit fiddly haha. Hope to finish that by this weekend though.

Also, I no longer have access to the ghpr-asia fork that this PR originally came from (I was originally interning at their company), so for the changes above I would need to open a new PR from my personal fork for the changes, which I will link to this PR (and probably close this one at that point).

Edit: removed "setters"; I just typed that out of habit haha

snproj commented 1 year ago

Just really quickly, as mentioned in the review

If you have a proto file with explicit defaults, there is a good chance you will have to type the code to access it so we might as well generate it for the user.

I realized that the getters will mainly be used for fields with explicit custom defaults, so I think we can generate getters for just those, at least for now.

I was trying to generate for fields with non-custom defaults too, but things like Cow<str> cause issues: I would think a getter for a field of type Cow<str> should return &Cow<str>; but if I want to take standard default Cow::<str>::default() into account it's not possible (returning reference to newly created value).

Other options I don't think are ideal 1. Return `Cow::`: Need to clone data 2. Return `&str`: possible, but you'd have to manually add `Cow` back in anyways 3. Create global `const DEFAULT_cow_str: Cow:: = Cow::Borrowed("");` and return a reference to that: Not nice haha

In the interest of time, I'll most likely work on generating for custom default fields only (i.e. Proto2 with explicit default tag) for this PR and we can expand later if needed.

thomaseizinger commented 1 year ago

If you have a proto file with explicit defaults, there is a good chance you will have to type the code to access it so we might as well generate it for the user.

I realized that the getters will mainly be used for fields with explicit custom defaults, so I think we can generate getters for just those, at least for now.

Yeah, I never expected getters for the other fields? If there is no explicit default defined, you can just access the field without any surprises right? Perhaps what would be good is to add some documentation to those fields, such that when you look at the generated code, you are reminded that for those you should use the getter to make sure the default is taken into account.

snproj commented 1 year ago

I've opened the new PR #247 with updates as a continuation since I no longer have access to the ghpr-asia fork; let's continue discussion there!