WIP: Async.Choice #744

Closed
wants to merge 4 commits into
from

Projects

None yet

7 participants

@forki
Contributor
forki commented Nov 28, 2015

This is a first draft of Async.Choice as proposed in
https://fslang.uservoice.com/forums/245727-f-language/suggestions/10575069-add-async-choice-to-fsharp-core

This function is written by @eiriktsarpalis and already in use in Paket, but IIRC he still wants to change some bits to make it fitting into FSharp.Core.
I added some very basic tests and hope to write more tests soon.

I know that adding stuff to FSharp.Core is not easy and therefor this is low priority and should only be merged the next time you consider to change the Fsharp.Core surface area.

This code is part of my FsAdvent post.

@forki
Contributor
forki commented Nov 28, 2015

strange. shouldn't a surface area test be red?

@mexx mexx and 1 other commented on an outdated diff Nov 28, 2015
...s/FSharp.Core/Microsoft.FSharp.Control/AsyncModule.fs
@@ -314,6 +323,84 @@ type AsyncModule() =
Assert.AreEqual(0, counter)
+#if FSHARP_CORE_PORTABLE
+// nothing
+#else
+#if FSHARP_CORE_NETCORE_PORTABLE
+// nothing
+#else
+
+ [<Test>]
+ member this.``Async.Choice takes first result that is <> None``() =
+ let returnFirstResult (n:PositiveInt) (i:PositiveInt) x =
@mexx
mexx Nov 28, 2015 Contributor

To avoid casting to int afterwards, one can write (PositiveInt n) (PositiveInt i).

@forki
forki Nov 30, 2015 Contributor

cool thx

@KevinRansom
Contributor

@forki we can add APIs to fsharp.core in the master branch no problem. We will need to change the fsharp.core version number for the next major release.

for VS2015 branch we will not add APIs to fsharp.core.

@forki
Contributor
forki commented Nov 29, 2015

@kevinransom do you know why this doesn't break the surface area tests?

@KevinRansom
Contributor

@forki Looking at it now.

@colinbull
Contributor

for VS2015 branch we will not add APIs to fsharp.core.

Can I ask a stupid question but why? Does this mean we potentially have different surface areas for vs2015 and mono/core clr etc??

@KevinRansom
Contributor

@colinbull Every new major release we have new API's. F# 3.0, F# 3.1 and F# 4,0 all introduced new language features and CORE APIs. fsharp.org refresh the fhsarp/fsharp repo, and then Xamarin will update the mono version of the compiler. So no ... we will not have different API's across the various platforms. However, there will be different versions of F# and fsharp.core in existence in the same way that there always has been.

I hope this clears things up ...

Kevin

@KevinRansom
Contributor

@forki
The testcase fails correctly. It looks as if runtests.cmd replaces the failing errorlevel with a success when sending files upto appveyor.

Just testing a fix.

@colinbull
Contributor

Ahh, yes of course. If I'd have thought about I already knew this.. Thanks for the clarification thou.

Sent from my iPhone

On 29 Nov 2015, at 18:17, Kevin Ransom (msft) notifications@github.com wrote:

@colinbull Every new major release we have new API's. F# 3.0, F# 3.1 and F# 4,0 all introduced new language features and CORE APIs. fsharp.org refresh the fhsarp/fsharp repo, and then Xamarin will update the mono version of the compiler. So no ... we will not have different API's across the various platforms. However, there will be different versions of F# and fsharp.core in existence in the same way that there always has been.

I hope this clears things up ...

Kevin


Reply to this email directly or view it on GitHub.

@KevinRansom
Contributor

@forki It now successfully fails.

@forki
Contributor
forki commented Nov 30, 2015

ok build now fails for the correct reason. thanks kevin

@KevinRansom
Contributor

Imagine a world where it's harder for tests to fail than to succeed. Briefly we lived in such a world.

@isaacabraham
Contributor

@KevinRansom so you mean the case of the "successful failure"?

@forki
Contributor
forki commented Nov 30, 2015

Kevin, I can't get the build working on my machine. It always fails in the
surface area tests. I guess it's testing against a GAC version. How can I
tell the appveyor build to use the correct (self-built) version?
On Dec 1, 2015 12:25 AM, "Isaac Abraham" notifications@github.com wrote:

@KevinRansom https://github.com/KevinRansom so you mean the case of the
"successful failure"?


Reply to this email directly or view it on GitHub
#744 (comment)
.

@KevinRansom
Contributor

I will look at it when I get home. However, the portable libraries have quite different version numbers from the shipping fsharp.core and so ... hopefully would never accidently bind to items in the GAC.

@KevinRansom
Contributor

@forki
appveyor-build.cmd only runs a subset of tests. It does that because Appveyor has a hard limit of 40 minutes for a ci session. Which is bit of a pain in the rear.

If you want to run the portable unit tests:

appveyor-build.cmd.cmd
cd tests
runtests release coreunitportable7
runtests release coreunitportable47
runtests release coreunitportable78
runtests release coreunitportabl259

If you run them after running appveyor-build.cmd then everything should be set up peachy

I do think it makes sense to run at least one of the portable tests in appveyor-build.cmd, so I will add 259 to it. Hopefully that won't bust the ci time budget

@enricosada
Contributor

@KevinRansom @forki about test suite and appveyor, i added #750 to run all test suites

@forki
Contributor
forki commented Dec 1, 2015

Just to clarify: my issue is that local appveyor-build.cmd (which is still the only build script we have right?) doesn't test against the self built fsharp.core but against some version that does not contain the choice method.

@enricosada
Contributor

@forki locally you need to run all steps inside DEVGUIDE - Full Steps Before Running Tests, so dll are built and RunTest.cmd use these, otherwise the assembly in gac are used.

if you run tests\config.bat ( used by RunTests.cmd ) you can check if if going to use gacced version or local build assemblies

FSCOREDLLNETCORE78PATH=C:\Program Files (x86)\Reference Assemblies\Microsoft\FSh
arp\.NETCore\3.78.4.0\FSharp.Core.dll

and yes, we need to improve the build script and add some helpers to make it easier to contribute

@forki
Contributor
forki commented Dec 1, 2015

I thought we already had this. sigh

@enricosada
Contributor

sry my bad, i was writing about debug configuration.
appveyor-build.cmd build in release ( so in release directory ) and run some tests, maybe are you running runtests debug instead of runtests release ( or visual studio test runner? )

@forki
Contributor
forki commented Dec 1, 2015

no I meant: yes I run only the appveyor-build.cmd, but I thought it already did the correct thing.

@enricosada
Contributor

it should (build release and run a subset of tests) maybe the referenced dll is wrong, i'll check this branch

@enricosada
Contributor

@forki sent a pr

@KevinRansom
Contributor

@forki @eiriktsarpalis

Guys,

"This function is written by @eiriktsarpalis and already in use in Paket, but IIRC he still wants to change some bits to make it fitting into FSharp.Core."

Would you like to rewrite the history of the PR a bit to correctly attribute erik for his contribution. Git does a good job of maintaining attribution for code. Alternatively ... and less desirably ... assert that you own the rights to the code, and are sharing it under apache 2.0. It's important that the licensing is correct.

Thanks

Kevin

@forki
Contributor
forki commented Dec 8, 2015

I can close this one if @eiriktsarpalis wants to resubmit.

@KevinRansom
Contributor

I'm happy for the api, it's just I would like to ensure attribution is correct.

@forki
Contributor
forki commented Dec 17, 2015

Closing in favor of #807

@forki forki closed this Dec 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment