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

Add ofNull and toNull to ValueOption #495

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

marner2
Copy link
Collaborator

@marner2 marner2 commented Jun 29, 2022

Add some helpers that allow for converting between ValueOption and nullable values without requiring that 'a be statically restrained as a nullable type. toNull will never generate ValueNone if the value is not nullable, and ofNull will throw an exception if you feed it a ValueNone when 'a is a non-nullable value.

I'm doing it unchecked currently, but I don't know of any way to match/discriminate on a type between nullable and non-nullable. I'm pretty sure you always end up with the type parameter being constrained all the way up, or not able to call constrained functions (hence why the implementation has to use boxing).

@TysonMN TysonMN changed the title Add ValueOption.ofNull and toNull Add ofNullable and toNullable to ValueOption Jun 29, 2022
@TysonMN
Copy link
Member

TysonMN commented Jun 29, 2022

Commit 942f30e is a refactor of what you originally shared.

After posting this comment, I will force push a commit that adds the null type constraint to the toNullable function. Then the function will not throw a NullReferenceException. (Some of the test code in the previous commit will not compile.)

Can your code in your other PR that makes use of toNullable be changed to satisfy this type constraint?

src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
Copy link
Contributor

@LyndonGingerich LyndonGingerich left a comment

Choose a reason for hiding this comment

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

These changes make the build work.

src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Show resolved Hide resolved
src/Elmish.WPF.Tests/UtilsTests.fs Outdated Show resolved Hide resolved
@marner2
Copy link
Collaborator Author

marner2 commented Jun 29, 2022

@TysonMN The whole point of this construct is to not have the type constraint (see PR description).

It becomes ofObj and toObj with boxing with it, which isn't worth a separate construct.

The reason we need something like this is because we need to support view model getters that work with both a nullable type or a non-nullable type, and there are certain cases where we know the type will be nullable (submodel) and we need control in elmish over whether it's null or not (especially submodelwin).

@marner2
Copy link
Collaborator Author

marner2 commented Jun 29, 2022

@TysonMN also ValueOption.toNullable already exists in FSharpCore (as well as ofNullable). That's why I used ofNull and toNull

@marner2 marner2 force-pushed the feature/add_ofNull_toNull branch 2 times, most recently from bbb2b97 to 79dc28e Compare June 30, 2022 01:30
@TysonMN
Copy link
Member

TysonMN commented Jun 30, 2022

The whole point of this construct is to not have the type constraint (see PR description).

I see that now. Sorry I missed that before.

@marner2 marner2 force-pushed the feature/add_ofNull_toNull branch 3 times, most recently from bb2c718 to dcd3c5a Compare July 1, 2022 00:10
@TysonMN TysonMN force-pushed the feature/add_ofNull_toNull branch from dcd3c5a to 019d50a Compare July 1, 2022 02:53
@marner2 marner2 force-pushed the feature/add_ofNull_toNull branch from 019d50a to 314fa78 Compare July 2, 2022 02:09
@marner2 marner2 changed the title Add ofNullable and toNullable to ValueOption Add ofNull and toNull to ValueOption Jul 5, 2022
@TysonMN TysonMN force-pushed the feature/add_ofNull_toNull branch from 314fa78 to 357c88c Compare July 7, 2022 02:02
@TysonMN
Copy link
Member

TysonMN commented Jul 7, 2022

The force push I just did mostly cleans up the code. I deleted one test for Int32 since there was already a test for int (and they are the same) and added one test for Nullable<int>.

@TysonMN TysonMN merged commit 481b36c into elmish:master Jul 7, 2022
@TysonMN
Copy link
Member

TysonMN commented Jul 7, 2022

Great work on this @marner2! I am very happy with the final implementation. I think the effort we put into this PR was worth it :)

@marner2 marner2 deleted the feature/add_ofNull_toNull branch July 8, 2022 20:59
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.

3 participants