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

Using a dedicated enum for the Try protocol #590

Closed
udoprog opened this issue Jul 29, 2023 · 2 comments · Fixed by #626
Closed

Using a dedicated enum for the Try protocol #590

udoprog opened this issue Jul 29, 2023 · 2 comments · Fixed by #626
Labels
enhancement New feature or request

Comments

@udoprog
Copy link
Collaborator

udoprog commented Jul 29, 2023

While looking over #587 I realized that I'm not a huge fan of the current return value for the TRY protocol.

To summarize, since the introduction of the TRY protocol (#544) it uses Result to indicate whether the short circuit or not. While this meshes well with the existing type system (Value::Result already exists and does basically what we want it to) it lacks clarity and becomes a bit messy with types such as Result<T, Result<(), Error>>.

Here I'm proposing we adopt the ControlFlow enum for this protocol.

Internally it also means that we should be able to represent it as a Value to the degree that at least the internal Vm operations are supported, which basically means conversion to and from ControlFlow<Value, Value>.

CC: @ModProg

@udoprog udoprog added the enhancement New feature or request label Jul 29, 2023
@ModProg
Copy link
Contributor

ModProg commented Jul 30, 2023

Makes sense, I used Result because it worked and already existed. I guess there is a reason std uses ControlFlow.

@udoprog
Copy link
Collaborator Author

udoprog commented Jul 30, 2023

To get an idea of how difficult it is to add support for an external enum, check out #591 where I've added Value::Ordering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants