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

WIP: Async.Choice #744

Closed
wants to merge 4 commits into from
Closed

WIP: Async.Choice #744

wants to merge 4 commits into from

Conversation

forki
Copy link
Contributor

@forki 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 forki force-pushed the choice branch 3 times, most recently from f354534 to 71982b0 Compare November 28, 2015 12:12
@forki
Copy link
Contributor Author

forki commented Nov 28, 2015

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


[<Test>]
member this.``Async.Choice takes first result that is <> None``() =
let returnFirstResult (n:PositiveInt) (i:PositiveInt) x =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool thx

@KevinRansom
Copy link
Member

@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
Copy link
Contributor Author

forki commented Nov 29, 2015

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

@KevinRansom
Copy link
Member

@forki Looking at it now.

@colinbull
Copy link
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
Copy link
Member

@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
Copy link
Member

@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
Copy link
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
Copy link
Member

@forki It now successfully fails.

@forki
Copy link
Contributor Author

forki commented Nov 30, 2015

ok build now fails for the correct reason. thanks kevin

@forki forki force-pushed the choice branch 2 times, most recently from bc6c391 to 6a05c25 Compare November 30, 2015 09:21
@KevinRansom
Copy link
Member

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

@isaacabraham
Copy link
Contributor

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

@forki
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Member

@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
Copy link
Contributor

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

@forki
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

forki commented Dec 1, 2015

I thought we already had this. sigh

@enricosada
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

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

@enricosada
Copy link
Contributor

@forki sent a pr

@KevinRansom
Copy link
Member

@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
Copy link
Contributor Author

forki commented Dec 8, 2015

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

@KevinRansom
Copy link
Member

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

@forki
Copy link
Contributor Author

forki commented Dec 17, 2015

Closing in favor of #807

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.

7 participants