Skip to content

refactor: Rename WithChannels, tx and rx #32

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

Open
wants to merge 19 commits into
base: Frando/spsc-is-mpsc
Choose a base branch
from

Conversation

Frando
Copy link
Member

@Frando Frando commented Jun 19, 2025

We have some reports that the WithChannels { tx, rx, inner } terminology is not very intuitive for people.

This very-much-breaking PR proposes the following renames:

  • WithChannels { inner, tx, rx, span } becomes Request { message, reply, updates, span }
  • Request is renamed to RequestSender (to be able to use Request for what was so far WithChannels)
  • derive arguments are updated accordingly: tx -> reply, rx -> updates

Let's discuss! We only wanna do this once.

cc @b5

@Frando Frando force-pushed the Frando/tx-rx-to-response-request branch from e915b84 to bbc2f6a Compare June 19, 2025 11:04
@rklaehn
Copy link
Collaborator

rklaehn commented Jun 19, 2025

Hm, the whole thing is the request, the inner thing is the message. But then I call the messages XXXRequest usually, e.g. GetRequest, PutRequest. Still, this part doesn't seem so controversial.

About rx/tx vs update/reply. It's longer and deviates from the async rust convention of tx/rx. Is it really that important that this is intiuitive vs short, given that it is intentionally pretty low level to match what you would do by hand?

I guess @b5 should comment. You are the target audience for this lib. If you find it much better I guess we should do it.

@b5
Copy link
Member

b5 commented Jun 19, 2025

I'm a big fan of updates as the word for streaming responses, but I think Request.reply should be Request.response, ditto for the derive tx -> reply, which I think should be tx -> response.

The struct being called Request, and a field of that struct containing a response does feel kinda strange. Generally I find this within "shrugging distance" that could be explained with a clarifying comment, but I think I need to play with implementing from this branch to be sure. Will try to run an experiment at some point in the next week & report back here.

To me any struct that's called Request should generally be the output of some sort of builder pattern, where I construct a request & call some kind of do on it, usually passing it to a Client that actually executes the request, and returns a new struct that defines the response, or a stream of responses.

@Frando
Copy link
Member Author

Frando commented Jun 19, 2025

I think I agree to what @b5 said, but also to the niceness of short names for these oftenly-destructured structs that @rklaehn brings up. Let's go back to the bikeshed board:

// status quo
let WithChannels { inner, tx, rx, span } = msg;

// frando's first suggestion
let Request { message, reply, updates, span } = request;

// b5's first suggestion
let WithChannels { request, response, updates, span } = msg;

I'm unsure myself. WithChannels always felt a bit weird for me, but I agree that Request is also imperfect. Any other ideas on what to call the "struct that contains the initial request message, and the streams for responses and updates from the client"?

So here's some more variations

let Incoming { request, tx, rx, span } = msg;
let ReqCtx { message, tx, rx, span } = ctx;
let Rpc { msg, res, req, span } = rpc;

@Frando Frando force-pushed the Frando/spsc-is-mpsc branch from 6eea8ed to 0c4fe47 Compare June 20, 2025 07:55
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.

3 participants