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

results: collections integration #179

Merged
merged 2 commits into from
Jun 7, 2023
Merged

results: collections integration #179

merged 2 commits into from
Jun 7, 2023

Conversation

arnetheduck
Copy link
Member

This set of helpers allows treating Result and Opt as collections of 0 or 1 item, allowing iterating over them and checking "membership" - such integration is useful in generic code which can then be generalised to handle more complex cases - the integration is most useful with Opt.

One design tradeoff here is the "explicitness" of items vs values for Result - technically error and value are "equal" and therefore we shouldn't give preference to the value, but there exists a convenience argument to treat the value as the "default" and therefore define items / contains for Result as well - this PR chooses the more conservative and explicit approach - a more liberal version can easily be added later should motivating examples emerge.

@zah
Copy link
Contributor

zah commented Apr 27, 2023

There are some dangers of overly generic code.

Consider the situation where a function foo returns Opt[seq[T]]. The users might accidentally write: for x in foo(): expecting to iterate over the sequence, but instead they will trigger the items iterator over the Opt.

Granted, this is likely to produce another error within the body of the loop, but with sufficient use of type inference the error message may turn out to be quite distant from the root cause.

Due to the way how generics are supposed to mixin symbols from their caller scope, there is an alternative way to provide this genericity as an opt-in. You can place the definitions in a separate module that users who are interested in this generic treatment of Results as collections can import in their code which calls the generic algorithms they want to use. Unfortunately, due to current issues such as the generic sandwitch, a solution like this would still be problematic in practice.

@arnetheduck
Copy link
Member Author

There are some dangers of overly generic code.

That generic code is not in Result however? Ie the code presented here is quite simple.

That said, "generic" here means converters, sequtils and other "functional"-style api:s that might become more popular now that iterators finally can be combined more naturally - these helpers in Result simply integrate Result with more parts of the language.

@zah
Copy link
Contributor

zah commented Apr 27, 2023

That generic code is not in Result however? Ie the code presented here is quite simple.

I don't understand this remark. Clearly the problematic example that I have provided will be possible with the current code.

@arnetheduck
Copy link
Member Author

arnetheduck commented Apr 27, 2023

You can place the definitions in a separate module

Yeah, the sandwitch issue is messy so for a central type like this, I would probably avoid it. I would probably use separate modules for anything that brings in additional imports (say .. std/options integration or I/O) but not for things that depend only on the Result type itself - from a UX perspective, this is often what you want as a developer, ie you don't want something to pull in deps you end up not using, but the dead-code-elimination feature of nim means the small helpers are "harmless" and convenient to always have available.

@arnetheduck
Copy link
Member Author

I don't understand this remark. Clearly the problematic example that I have provided will be possible with the current code.

sure - what I meant was that the judgement of "what kind" of code to use with these features is on the user, the code inside the Result module is not "overly generic", ie it doesn't contain too-open generics that would accidentally have negative effects on surrounding code.

and yeah, if someone forgets to dereference in the for loop, that should quickly become apparent - it's like when usesing seq[seq[.. which would cause the same kind of error messages.

combining functional helpers is an incredibly powerful technique in languages that support it well - it "fits" Result that way, though the lack of good functional support in Nim in general has so far hampered progress on this front.

@zah
Copy link
Contributor

zah commented May 11, 2023

Opt[seq] is likely to be more common than seq[seq], but my comment didn't meant to block this PR from merging - merely to raise awareness of the potential risk. We can give it a try and see how things go in practice.

This set of helpers allows treating Result and Opt as collections of 0
or 1 item, allowing iterating over them and checking "membership" - such
integration is useful in generic code which can then be generalised to
handle more complex cases - the integration is most useful with Opt.

One design tradeoff here is the "explicitness" of `items` vs `values`
for `Result` - technically error and value are "equal" and therefore we
shouldn't give preference to the value, but there exists a convenience
argument to treat the value as the "default" and therefore define
`items` / `contains` for `Result` as well - this PR chooses the more
conservative and explicit approach - a more liberal version can easily
be added later should motivating examples emerge.
@arnetheduck arnetheduck merged commit 13e55ed into master Jun 7, 2023
@arnetheduck arnetheduck deleted the res-coll branch June 7, 2023 14:45
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.

2 participants