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 syntax for Supervisor #2068

Closed

Conversation

armanbilge
Copy link
Member

I am quite lazy. This PR adds syntax F[A]#supervise equivalent to Supervisor[F]#supervise(fa: F[A]) which is available whenever an implicit Supervisor[F] is in scope. Not sure where this would go, so I created a syntax package in std.

@vasilmkd
Copy link
Member

This looks good to me. Should we port it to CE2 as well?

@vasilmkd
Copy link
Member

Unless there are objections to passing Supervisor instances implicitly. Not sure if we have a firm decision on this.

@armanbilge
Copy link
Member Author

armanbilge commented Jun 20, 2021

Unless there are objections to passing Supervisor instances implicitly. Not sure if we have a firm decision on this.

Discussing on discord, @ChristopherDavenport made this suggestion:

Or if you are in the mood to live dangerously import supervisor.syntax._ in which case if the syntax is used it references that supervisors instance.

Although there may be some trickiness regarding the import. Update: see this example
https://scastie.scala-lang.org/3gjiuAaaRoeEclAUuIcMJg

@djspiewak
Copy link
Member

I'm not really a fan of passing Supervisor implicitly, for the same reason I don't think it's a good idea to pass Dispatcher implicitly. Also neither of them are really set up for the "summoning" pattern. With that said, that doesn't really prevent the syntax, it would just mean that the caller would need to pass the supervisor explicitly: fa.supervise(supervisor). Is that worthwhile? It does add to the discoverability, at least in theory.

@vasilmkd
Copy link
Member

Personally, I see Dispatcher and Supervisor as similar constructs to CE2's Blocker, which wasn't passed around implicitly. Maybe we should not go through with this change.

@armanbilge
Copy link
Member Author

Ok, sounds like this isn't a great idea after all. I'm happy to adapt my PR for @djspiewak's suggestion fa.supervise(supervisor), otherwise we can close. Thanks for considering it though!

@djspiewak
Copy link
Member

Closing this for now; I think the revised syntax is worthwhile, but let's get it into its own PR!

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