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

Suggestion - Change behavior of select #22

Closed
MichaelAz opened this issue Jun 21, 2014 · 6 comments
Closed

Suggestion - Change behavior of select #22

MichaelAz opened this issue Jun 21, 2014 · 6 comments

Comments

@MichaelAz
Copy link
Contributor

I've been playing with the library and I'm enjoying it immensely, but there's one thing that bugs me - why does select take a list? Sure, that's the way go does it but it's much more Pythonic IMO for it to take an *args argument.

A second point, of which I'm less sure, is that it seems the case used most often for select is rcase, that is, waiting on a result from a channel. I think it would be nice if I could pass a channel object and get it wrapped with an rcase as a default behavior.

So, to recap, I propose:
case, val = goless.select(c1, c2)

Instead of:
case, val = goless.select([goless.rcase(c1), goless.rcase(c2)])

Also, since this would make that switching pattern suggested in the docs less elegant to implement, complementary is_receive, is_send and is_default methods could be added for testing the case return value.

If this sounds good to you, I'd be happy to make a pull request with the suggested changes.

@chuim
Copy link
Collaborator

chuim commented Jun 23, 2014

Hi Michael and thanks for the suggestion. I agree with your points and I think it could be an improvement indeed.

Furthermore, I personally don't really like our current naming choices for the cases -- rcase, scase, dcase -- for I don't think they are a tad cryptic. One has to look at the docs to understand what each of them is. So I'm also looking at some other options to improve both the Python-icity and legibility.

Anyway, we'll have to wait on @rgalanakis for a final position though.

@rgalanakis
Copy link
Owner

  • The idea behind select taking a list is that the select normally happens in a loop, so rather than construct the list each time, callers should construct it once and keep using it. But I have no problem using a *args as well. I'd ask that the old behavior be preserved: if the first item is a list, it should be the only argument. It makes the implementation slightly more complex, and there need to be a number of new tests, but IMO it is better for the caller. Happy to take a PR for this one.
  • Feel free to alias dcase, scase, and rcase to default_case, send_case, and recv_case. Please make a new issue and submit a PR, or @chuim just submit it to master.
  • I'm much less sure about the implicit channel conversion to rcase. I feel it ends up complicating things more than simplifying (new methods and a totally different pattern for selecting). I'd need to see some examples of where this simplifies code. I am open to other solutions. A select_recv method that takes channels? An rcase.make(*channels) classmethod that returns a list of rcase instances (so you'd call goless.select(*rcase.make(c1, c2, c3))? But really I would want to hone in on the problem first by seeing some example code.

Thanks for the feedback!

@chuim
Copy link
Collaborator

chuim commented Jun 26, 2014

Based on this example from our test suite:

cases = [goless.rcase(self.chan1), goless.dcase()]
result, val = goless.select(cases)

I was trying out some ideas inspired by fluent interfaces. Without caring too much to naming, this is what a select statement would look like:

cases, result, val = goless.buildCases().add_receive(self.chan1).add_default().select()

But when writing the tests I realized that the "look-and-feel" that I wanted to achieve (look at the 1st example from the Wikipedia link above) is not practical in Python because of how line-breaks are handled.

So I tried using context managers that would allow something like:

        with goless.buildCases() as b:
            b.add_receive(self.chan1)
            b.add_default()
            cases, result, val = b.select()  # or tabbed back, it doesn't matter

But this looks too verbose and doesn't look like it improved anything.

So I guess our current solution with the addition of the *args as suggested by @MichaelAz should look better than both of these.

What do you all think?

EDIT: Note: I know that I'm abusing/misusing the with clause there. I just thought it might look closer to what an actual go select block looks.

@MichaelAz
Copy link
Contributor Author

I've created a PR for the new select syntax.
Regarding the aliasing - I actually like rcase, scase and dcase because they're short and terse.
Regarding the channel\rcase issue - The select_recv idea sounds nice, although I think a better name would be recieve.

@rgalanakis
Copy link
Owner

Okay. @chuim, feel free to alias scase/rcase/dcase if you want.
@MichaelAz, if you end up finding the select_recv/select_receive functions useful, I'd love some more information.
Thanks again!

@rgalanakis
Copy link
Owner

Closing this as the main outcome has been address in #23. Add new issues if anyone wants to pursue the other topics.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants