Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(codec): Enforce encoders/decoders are Sync #84

Merged
merged 5 commits into from
Oct 30, 2019

Conversation

LucioFranco
Copy link
Member

cc @NeoLegends

Closes #81

@LucioFranco
Copy link
Member Author

Yeah...this will be a bit harder than anticipated...

@NeoLegends
Copy link

NeoLegends commented Oct 21, 2019

Yeah, this is the other option. I guess you're in the best position to know which bounds you will be able to satisfy in tonic.

For this implementation route we'd also want to add a test that ensures Streaming stays sync (when adding new members).

You might also want to add a Closes #82 to close the other PR automatically when this one is merged. I'll do the opposite on the other one.

@NeoLegends NeoLegends mentioned this pull request Oct 21, 2019
@LucioFranco
Copy link
Member Author

@seanmonstar would you be able to take a look at this?

@seanmonstar
Copy link
Member

What I'm about to say sounds dumb: for trait objects, the order of additional auto traits matters, and most places write them as dyn Foo + Send + Sync. That's to say, dyn Foo + Sync + Send isn't the same thing. 😭

@LucioFranco
Copy link
Member Author

@seanmonstar all i can say is lol

@NeoLegends
Copy link

Does this put #82 back on the table?

@LucioFranco
Copy link
Member Author

@NeoLegends we still want to go with this one but need to change the order of the bounds.

@LucioFranco
Copy link
Member Author

@seanmonstar can I get your +1 on this before we merge?

@LucioFranco LucioFranco merged commit 3ce61d9 into master Oct 30, 2019
@LucioFranco LucioFranco deleted the lucio/streaming-sync branch October 30, 2019 19:00
rabbitinspace pushed a commit to satelit-project/tonic that referenced this pull request Jan 1, 2020
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider making Streaming<T> Sync
3 participants