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

Option should implement Seq #716

Closed
charlesroddie opened this Issue Jan 21, 2019 · 10 comments

Comments

Projects
None yet
6 participants
@charlesroddie
Copy link

charlesroddie commented Jan 21, 2019

Options should implement Seq

for u in (o:User option) do print ("Hello " + u.Name)

It's common to do something with an Option when it exists, which is why we have Option.iter, as a friend of List.iter, Array.iter, Seq.iter.

For lists, arrays, sequences, and options, for x in s do .. is intuitive and concise syntax, often preferable to s |> ***.iter (fun x -> .. ). It's useful to have both approaches.

This syntax is also allowed inside sequence comprehensions: [ for x in s do yield x ** 2 ] is alowed while [ s |> ***.iter (fun x -> yield x ** 2 ) ] is not.

Options are missing the for x in s do .. syntax because they don't implement Seq.

Pros

Cleaner syntax for doing things with options that makes brings the iteration facilities for Options in line with List and Array.

Having Option.iter implies you can iterate over an option, and it's confusing that the other syntax for iteration isn't there. Users have to remember that the functionality is not there.

Cons

None that I can see.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions:
#705

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [ x ] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [ x ] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [ x ] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [ x ] This is not a breaking change to the F# language design
  • [ x ] I or my company would be willing to help implement and/or test this
@charlesroddie

This comment has been minimized.

Copy link
Author

charlesroddie commented Jan 21, 2019

Responses from linked issue:

@abelbraaksma I like that idea, and a simple implementation of an Option.toSeq would solve that.

@theprash
I really like this! @bruinbrown has just pointed out to me that you could also then write this:
yield! opts.Id |> Option.map Props.Id
Is there some other pitfall of making Option directly implement seq?

@cartermp I don't think I'd support having Option implement Seq. It's not a sequence of (potentially infinite) items, it's either something or nothing. It also feels weird to me that for id in ... implicitly pattern matches the value out, if there is one.

@cartermp

This comment has been minimized.

Copy link
Member

cartermp commented Jan 22, 2019

I'd like to see what @alfonsogarciacaro and @MangelMaxime have to say, since the pattern where this could help may be useful in Fable given the issue this was created from. Although I'm personally still against it, I'm not qualified to speak from the Web UI side of things.

@MangelMaxime

This comment has been minimized.

Copy link

MangelMaxime commented Jan 22, 2019

Thanks for pinging me @cartermp I first took a lot at #705 and I don't think the proposed synthax add value. I mean if people find it to verbose to do Option.IsSome opts.Id we could probably rewrite it to opts.Id.IsSome.

For the current issue, do you or @charlesroddie have an example with a Web UI code. Because I kind of fail to see where it should be use for UI declaration.

I say that because in general, when using react we try to avoid using yield and yield! because doing that we are hidding from react that we are working from a list and so limit its optimisation phase.

When working with list in views, I recommand to use ofList, ofArray helpers from react package which helps react identify list and help him optimize the diff.

@alfonsogarciacaro

This comment has been minimized.

Copy link

alfonsogarciacaro commented Jan 22, 2019

From Fable's perspective this would bring a lot of issues, because right now options are erased by Fable in the runtime (in most cases). One of the main reasons for this decision was that Option is used in F# to represent optional arguments and we had to remove None arguments when interacting with native JS functions. Because of this, it won't be possible to attach any the IEnumerable interface (transformed to JS Iterable in runtime by Fable) to None elements.

I hadn't seen the #705 proposal yet, but I actually like it much more as using match x with Some x -> ... | None -> () is not nice and accessing .Value in options doesn't feel quite right (although it should be safe after checking if Some). The main issue with Option.iter (besides being also verbose and not compatible with list comprehensions) is it creates an extra closure, although we could try to optimize this in the compilation.

@cartermp

This comment has been minimized.

Copy link
Member

cartermp commented Jan 22, 2019

I think that this proposal does the following for an option:

  • When it is None, gives ()
  • When it is Some x, creates a seq<'T> for that single value

In some sense, it is a different way to pattern match when you want to do something effectful and it does let you combine that with yield/yield! for heavy expression-based code.

Not sure if that affects if this is possible in Fable or not.

@MangelMaxime

This comment has been minimized.

Copy link

MangelMaxime commented Jan 22, 2019

Ok, for me from a Fable side I don't see much benefit of adding it.

But I am not a heavy user of yield/yield!.

@bruinbrown

This comment has been minimized.

Copy link

bruinbrown commented Jan 22, 2019

For some context of why I mentioned this to @theprash the other day, after I was shown the initial linked proposal, I realised it applied quite nicely to what I was doing in Fable where I have 3 options to build up a UI. I had something similar to the following:

...
div [] [
    match previous with
    | Some x -> yield x
    | None -> ()
    match current with
    | Some x -> yield x
    | None -> ()
    ...
]

By having Option implement Seq then it would have been possible to instead do the following

div [] [
    yield! previous
    yield! current
    yield! next
]
@MangelMaxime

This comment has been minimized.

Copy link

MangelMaxime commented Jan 22, 2019

@bruinbrown Yes, the code you are showing is exactly what I was speaking about.

You sould avoid (never use) yield! in your views but instead use ofOption, ofList, etc.

This talk explains why we should not use yield! in the views.

If people want to add this feature mostly/only for Fable views, then I would say we shouldn't because it prone a bad way to write views :)

@charlesroddie

This comment has been minimized.

Copy link
Author

charlesroddie commented Mar 2, 2019

@cartermp I think that this proposal does the following for an option:
When it is None, gives ()
When it is Some x, creates a seq<'T> for that single value

Sorry I missed this comment.
That isn't the idea and wouldn't work well at all if some values don't generate a Seq and others do.
None should create an empty seq<T>.

Also can the Fable-inspired tag be removed here? I don't know much about Fable but I don't think it's very connected.

@dsyme

This comment has been minimized.

Copy link
Collaborator

dsyme commented Mar 9, 2019

This is in the category of "decided in F# 1.0", so I'll close this per the guidelines.

On .NET Option.None is represented as null, so implementing the interface is not possible unless all consumers know to interpret null as the empty sequence.

So there are problems both on Fable and .NET

@dsyme dsyme closed this Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.