-
Notifications
You must be signed in to change notification settings - Fork 789
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
FS-1135 implementation - random functions for collections #17277
Conversation
❗ Release notes required
|
Can somebody please help me pass unit tests? It fails with |
I think you might want to run something like this $env:TEST_UPDATE_BSL=1
>> dotnet test tests/FSharp.Core.UnitTests/FSharp.Core.UnitTests/ -c Release --filter "SurfaceArea" That should update the baselines (sometimes you may need to run it twice), which you can then commit. |
Thanks it worked, just as I figured out it should be |
Ah, yeah, whoops |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
@abelbraaksma you are welcome to review as well! |
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
Thanks for this, great effort and great docs.
I haven't looked into the implementation yet, I was rather checking if the API adheres to the RFC - which it does, apart from one exception type in one case.
I will finish my review soon. Overall, I am up for merging this in its current shape. I think this API is good enough. It could probably be made thinner or more discoverable, as per discussions in the related threads. But unless it proves to be a performance trap AND cannot be optimized within the API boundaries in followups - I don't think it's worth starting the design process all over again.
tests/FSharp.Core.UnitTests/FSharp.Core/Microsoft.FSharp.Collections/ListModule.fs
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 2 pipeline(s). |
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.
Good stuff, I am for getting this in (see my comment above).
The implementation LGTM, just give us some time to figure out all this exception naming stuff, also the CI is broken right now.
Alright, let's get this in. Thanks for your work @Lanayx. Let's spread the word :) |
Description
FS-1135 implementation
Checklist
Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated: