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

Add #[postcard(bound = "...")] attribute for derive(Schema) #154

Merged
merged 4 commits into from
Dec 23, 2024

Conversation

ia0
Copy link
Contributor

@ia0 ia0 commented May 14, 2024

This doesn't fix #153 but provides a work-around (which is also present in serde).

Copy link

netlify bot commented May 14, 2024

Deploy Preview for cute-starship-2d9c9b canceled.

Name Link
🔨 Latest commit c0c1ded
🔍 Latest deploy log https://app.netlify.com/sites/cute-starship-2d9c9b/deploys/664370b45494810008cb7317

Copy link

netlify bot commented Dec 22, 2024

Deploy Preview for cute-starship-2d9c9b canceled.

Name Link
🔨 Latest commit 3b57a69
🔍 Latest deploy log https://app.netlify.com/sites/cute-starship-2d9c9b/deploys/676983bd8b7c0c0008bc2be6

@max-heller
Copy link
Collaborator

max-heller commented Dec 22, 2024

Hi @ia0, I merged this with main since things have changed quite a bit since you opened the PR. Could you could add a section to the docs here describing the attribute?

@jamesmunns have you thought any more about the future of MaxSize? Currently, this only affects derive(Schema), but if we want to keep MaxSize as a derive, we should probably have derive(MaxSize) take this attribute into account or provide fine-grained attributes like #[postcard(bound(schema = "...", max_size = "..."))] (mirroring #[serde(bound(serialize = "...", deserialize = "..."))]).

@ia0
Copy link
Contributor Author

ia0 commented Dec 23, 2024

Could you could add a section to the docs here describing the attribute?

Yes. However, note that I probably won't be able to spend too much time on this PR since I ended up writing and using my own serialization library wasefire-wire.

@max-heller
Copy link
Collaborator

However, note that I probably won't be able to spend too much time on this PR since I ended up writing and using my own serialization library wasefire-wire.

I think this is pretty much good to go, thanks for the contribution and the pointer to wasefire which looks quite interesting!

@jamesmunns
Copy link
Owner

@max-heller I haven't had a chance yet! I may have some time on postcard over the next two weeks. I'm inclined to favor the const fn version, but I know there are open design points on that as well right now. I wouldn't object to extending this change to MaxSize, but also fine if it stays as-is for the moment.

@ia0 I'm not sure if you've done a side-by-side with wasefire and postcard-rpc, I'd definitely be interested to help set one up to understand the differences in design you ended up with! I remember the use cases being pretty overlapping? Happy to coordinate in an issue or email if you think it would be useful.

@max-heller max-heller merged commit f1c89db into jamesmunns:main Dec 23, 2024
5 checks passed
@ia0 ia0 deleted the schema branch December 24, 2024 09:41
@ia0
Copy link
Contributor Author

ia0 commented Dec 24, 2024

I'm not sure if you've done a side-by-side with wasefire and postcard-rpc, I'd definitely be interested to help set one up to understand the differences in design you ended up with! I remember the use cases being pretty overlapping? Happy to coordinate in an issue or email if you think it would be useful.

No I didn't do anything precise because wasefire is much more opinionated (wasefire-wire is not really meant to be used outside wasefire even if the wire format is postcard compatible). The only question is whether postcard-rpc covers what wasefire-protocol needs. I don't know how much the schema changed since last time I checked, but that was the main issue. There was also things I didn't like about serde, but I don't remember how much of an issue it was. It's true that it would be good to check this at some point. I've created google/wasefire#708 but probably won't be able to look at it before late 2025.

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.

derive(Schema) introduces incorrect bounds instead of doing perfect derive
3 participants