-
-
Notifications
You must be signed in to change notification settings - Fork 747
Add std.range.Option #3915
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 std.range.Option #3915
Conversation
48af533 to
1c15467
Compare
|
|
I also have these: Option!T left(T, U)(auto ref Algebraic!(T, U) algebraic)
{
if(auto p = algebraic.peek!T)
return some(*p);
else
return option(none);
}
Option!T right(T, U)(auto ref Algebraic!(T, U) algebraic)
{
if(auto p = algebraic.peek!U)
return some(*p);
else
return option(none);
}Not sure if I should include them in this PR. edit: I guess an API that adds an |
std/typecons.d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be moved to std.traits? If they are helpful here then the chances of them being helpful to someone else is high.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's outside the scope of this PR. They can be moved later. (I think std.allocator also has its own isCopyable template)
14ec3a5 to
d17d365
Compare
|
Fixed linker error and added Option.orElse. |
d17d365 to
efe8495
Compare
|
|
std/typecons.d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
foreach
Why not use $(D foreach). I thought that that was the standard. Same applies everywhere else
$(REF each, std, algorithm, iteration)
this didn't link for some reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
… is the same as $(D …).
REF is from dlang/dlang.org/pull/1184.
|
Is there any reason you don't specialize |
std/typecons.d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is inout not usable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it is not, that's why it's structured like it is.
(edit: this way also makes the Algebraic import lazy, while still keeping it in the documentation as the return type)
|
@MetaLang, this way supports edit: e.g.: auto arr = [null, new Object];
auto maybeFront = arr.frontOption;
assert(maybeFront.assumeNonEmpty == null); |
std/typecons.d
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const and immutable overloads are untested because Algebraic does not support non-mutable types...
efe8495 to
4b8f5b5
Compare
|
What is the relationship between this and |
|
Most of the work here is not duplicated by I think the best way would be to make unary |
4b8f5b5 to
f7e13ea
Compare
|
Fixed compilation failure with latest DMD. |
f3c62a6 to
938deb5
Compare
|
Added |
|
I'm not convinced that I would much prefer all access to the value is null/assert-safe or uses some clear method like
I think a version that returns an |
Range code that assumes non-empty is wrong. Don't use Option being a range is the whole point. It's the same in Scala where it has been very successful.
How would |
How do you modify |
std/typecons.d
Outdated
|
|
||
| auto values = only(false, true, false).map!getX.map!option; | ||
| assert(values[0].empty); | ||
| assert(!values[1].empty && *values[1].front == 42); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use Option.front directly either, it's just for passing to range algorithms.
Please replace front here with something else (as this appears in the docs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I ate my own dog food and rewrote it to use equal.
|
Yeah, you're right, this is too similar to unary I noticed that @ntrel, the current |
938deb5 to
53e157b
Compare
ccb42c9 to
33facb1
Compare
Modeled after Scala's scala.Option type. Fix ticket 12679
33facb1 to
c778381
Compare
About About auto aa = ["foo": 42];
assert(aa.lookup("foo").equal(only(42)));
// instead of
assert(option("foo" in aa).map!(p => *p).equal(only(42)));This is the same inconvenience faced by any function that returns a null pointer to mean absence of value, such as No-one has said anything about it, but this PR still includes the arguably cargo-culty I'm aware of the build failure, I'll investigate. |
I suppose
Yes, we could have: Option!T deref(T)(T*);
Option!T deref(T)(Option!(T*));(I still think |
|
Alright, here are the real differences between
Code in std.path relies on |
|
|
||
| auto values = only(false, true, false).map!getX.map!option; | ||
| assert(values[0].empty); | ||
| assert(values[1].map!(p => *p).equal(only(42))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add an assert for values[1].equal(only(getX(true))) before this line. (And if we add deref, we can use it instead of map here and on line 7044).
BTW I'm working on edit: See #4363 |
|
Merge failed - needs rebase :) |
|
rebase pls |
Are u still alive @JakobOvrum? ;-) |
|
I see one way to push this forward: add a |
|
No activity from PR author in nine months. Closing. |
|
Convert this to an enhancement request? |
Could you imagine renaming |
|
Hmmm... neither name is very descriptive. The type really means "ZeroOrMore" whereas "Only" suggests exactly one and "Option" and "Optional" imply "zero or one". "Maybe" is arguably better. "Some" would be great but evokes "one or more" and promises to be a great source of confusion with Haskell's some. The same link suggests that "many" would be "zero or more". "Bag" is a term I'd like - you have a "bag" of things, which may be empty, contain one item, or more items. It's also a bummer that |
|
Oh, I didn't realize that |
Modeled after Scala's scala.Option type. It's been discussed on the forums before, I found at least this thread.
(I'll move it to std.experimental.typecons once #2945 is merged, as it establishes the module)