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

Provided implementation for Set.exactlyOne and Set.tryExactlyOne #9607

Closed
wants to merge 4 commits into from

Conversation

yreynhout
Copy link

@yreynhout yreynhout commented Jul 1, 2020

I only had the following insight after submitting this PR - I probably can't just change / extend the api surface of FSharp.Core. Still, it felt like this was missing. Some guidance on how to proceed would be appreciated.

@dnfadmin
Copy link

dnfadmin commented Jul 1, 2020

CLA assistant check
All CLA requirements met.

@abelbraaksma
Copy link
Contributor

abelbraaksma commented Jul 1, 2020

I assume you're adding this for parity with lists, sequences etc, right? Or is there a language suggestion? Personally, I think it's good to have it (parity, I mean).

I assume the reason tests fail is because if the surface area (the public functions) changes, they will fail. It's to protect against accidental additions of public functions, that would otherwise go unchecked.

@yreynhout
Copy link
Author

@abelbraaksma indeed, felt a bit weird that I had to either write custom functions or convert to a list to get the functionality.

@yreynhout
Copy link
Author

@cartermp Thanks for the hint. Are you suggesting I simply change the surface area in the test file(s)?

@cartermp
Copy link
Contributor

cartermp commented Jul 2, 2020

The surface area changes are necessary to make this mergeable. But I think that some new tests for the Set module are also in order here.

@abelbraaksma
Copy link
Contributor

FYI, the related suggestion for Seq and friends was: fsharp/fslang-suggestions#137. It should perhaps be paired with the suggestion to extend the surface area of Set: fsharp/fslang-suggestions#803.

I don't know if it is needed, but if so, I can help write up a short RFC for this. Or we could perhaps extend this PR to the other functions mentioned in fsharp/fslang-suggestions#803.

@yreynhout
Copy link
Author

@cartermp I'm more than willing to add tests but failed to find a suitable place. Any pointers as to where you'd like me to add them would be useful - I'm a newbie in this codebase. I do hope I can confine the tests to cover only what I'm adding.

@abelbraaksma If there is protocol to be followed I'll take any suggestions on how to proceed. I can copy the seq related RFC for tryExactlyOne and work my way from there.

@abelbraaksma
Copy link
Contributor

Sounds good, I'm not sure what the protocol is here, but my guess would be that changes to the API need an RFC, so that people are aware it exists, and/or can comment on the design.

@KevinRansom
Copy link
Member

@yreynhout , there is supposed to be a test that fails when you change the surface area of FSharp.Core. But yes, you can add api's to FSharp.Core, we just have to figure out if we want to make a change.

There is a Documented Change Request process here:https://github.com/fsharp/fslang-suggestions/issues

And here is an example: fsharp/fslang-suggestions#873

@brettfo brettfo changed the base branch from master to main August 19, 2020 20:01
@TIHan TIHan added Needs-RFC Area-Library Issues for FSharp.Core not covered elsewhere labels Aug 25, 2020
@KevinRansom
Copy link
Member

@yreynhout ,
I am closing this PR, because the API has not yet been approved in principle at: https://github.com/fsharp/fslang-suggestions/issues

Please pursue the RFC process mentioned and above and then reactivate this PR for consideration once the API additions have been approved in principle.

Thanks

Kevin

@abelbraaksma
Copy link
Contributor

abelbraaksma commented May 19, 2024

This could be part of the Set module expansion issue: fsharp/fslang-suggestions#1361. Linking for posterity ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Library Issues for FSharp.Core not covered elsewhere Needs-RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants