Remove second internal ValueOption #5717
Conversation
|
I believe that System.ValueOption was introduced with FSharp.Core 4.5, that would mean that the FCS if this change is accepted would require at least FSharp.Core v4.5 in-order to run. I am not certain of the targeting philosophy of FCS, but in the back of my mind, I remember that it was aimed at earlier versions of FSharp.Core: |
|
I think @dsyme suggested this change somewhere. I just remembered and did it
Kevin Ransom (msft) <notifications@github.com> schrieb am Mo., 1. Okt.
2018, 16:50:
… @forki <https://github.com/forki>,
I believe that System.ValueOption was introduced with FSharp.Core 4.5,
that would mean that the FCS if this change is accepted would require at
least FSharp.Core v4.5 in-order to run. I am not certain of the targeting
philosophy of FCS, but in the back of my mind, I remember that it was aimed
at earlier versions of FSharp.Core:
I.e.
https://github.com/Microsoft/visualfsharp/blob/master/fcs/FSharp.Compiler.Service/FSharp.Compiler.Service.fsproj#L638
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5717 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNEaG9840lMmr6EePiVY8dfOvQTjiks5ugiu2gaJpZM4XB2Iv>
.
|
|
@forki, I can well imagine he did. Well then FCS needs updating too. Which also explains the ci_part4 build failures I expect. Kevin |
|
Thanks for this Kevin |
| @@ -3090,7 +3090,7 @@ and OptimizeBinding cenv isRec env (TBind(vref, expr, spBind)) = | |||
| | None -> false | |||
| | Some mbrTyconRef -> | |||
| // Check we can deref system_MarshalByRefObject_tcref. When compiling against the Silverlight mscorlib we can't | |||
| if mbrTyconRef.TryDeref.IsSome then | |||
| if ValueOption.isSome mbrTyconRef.TryDeref then | |||
There was a problem hiding this comment.
Please, we should have defined an .IsSome property on ValueOption before accepting this change.
Part of the whole point of property syntax is that it creates far less churn when changing type.
|
This change was much more churn than needed - we should have just defined and almost no other changes would have been needed. |
|
IMO we should have a separate PR that adds these functions to the public API surface area to match option: https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/option.fs#L8 Doesn't help with churn, but if there is need for it in the compiler then it should probably be added. I expect an RFC is in order for that though. |
|
I didn't want to change surface area at that time. But extension methods
would have been possible.
Phillip Carter <notifications@github.com> schrieb am Di., 9. Okt. 2018,
17:26:
… IMO we should have a separate PR that adds these functions to the public
API surface area:
https://github.com/Microsoft/visualfsharp/blob/master/src/fsharp/FSharp.Core/option.fs#L8
Doesn't help with churn, but if there is need for it in the compiler then
it should probably be added. I expect an RFC is in order for that though.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5717 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNHy3ZLyLDCrikSeRXphXMjboGdhKks5ujMA-gaJpZM4XB2Iv>
.
|
No description provided.