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 output type information to BindingData #470

Merged
merged 1 commit into from
Sep 7, 2022

Conversation

marner2
Copy link
Collaborator

@marner2 marner2 commented Jun 2, 2022

This Pull Request adds another type parameter to BindingData which allows it to carry the actual view model property type statically through the builder.

From a conversation with @TysonMN:

When used in the context of "dot" notation in the XAML project, you will be able to get compile-time help for things like ViewModelProp.SubModel.Prop. If those were constrained to obj, you wouldn't get compile-time checking of those types.
Also XAML will warn you about type issues (number vs string, etc)

Note that the current usage of Binding<'model,'msg> constrains the new type parameter to be obj for every construction of it (this is the entire public interface in Binding.fs). This is intentional, as all of those bindings are by necessity constrained to obj in order to work in the various SubModel and SubModelSeq bindings. The advantage mentioned above will not be realized until after a static helper is created that can turn a binding into a statically-typed getter or setter.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 6ca2c75 to 3def63e Compare June 2, 2022 21:29
@TysonMN

This comment was marked as outdated.

@TysonMN

This comment was marked as outdated.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch 2 times, most recently from 7041541 to 6679f76 Compare June 4, 2022 17:12
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 6679f76 to e756d78 Compare June 7, 2022 03:30
@TysonMN

This comment was marked as outdated.

@marner2

This comment was marked as outdated.

@TysonMN

This comment was marked as outdated.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch 2 times, most recently from 009d843 to ed877dc Compare June 10, 2022 17:26
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from ed877dc to 613118e Compare June 13, 2022 03:51
Copy link
Member

@TysonMN TysonMN left a comment

Choose a reason for hiding this comment

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

Both the GutHub App and FastHub seem to be showing me weird white space changes. I want to review this on my laptop.

src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
Copy link
Member

@TysonMN TysonMN left a comment

Choose a reason for hiding this comment

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

Not part of this PR:
On line 669 of BindingVmHelpers, lets rename x to result

src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
@LyndonGingerich

This comment was marked as outdated.

@marner2 marner2 marked this pull request as draft June 14, 2022 02:15
@marner2

This comment was marked as outdated.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch 2 times, most recently from c48e014 to 4186415 Compare June 14, 2022 04:17
@marner2 marner2 marked this pull request as ready for review June 14, 2022 04:18
@marner2

This comment was marked as outdated.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 4186415 to 3a49795 Compare June 14, 2022 17:06
@marner2 marner2 marked this pull request as draft June 14, 2022 17:07
@marner2

This comment was marked as outdated.

@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 3a49795 to 1bfdd41 Compare June 14, 2022 23:12
@marner2 marner2 force-pushed the feature/add_vm_type_information branch 4 times, most recently from 58dfbab to 86de7e3 Compare July 22, 2022 17:57
@marner2
Copy link
Collaborator Author

marner2 commented Jul 22, 2022

@TysonMN I put #498 before this one now, since I was rebasing a lot. That one now needs to be merged in first

@marner2 marner2 force-pushed the feature/add_vm_type_information branch 3 times, most recently from 9567dd7 to 8dfa361 Compare July 26, 2022 21:05
@marner2 marner2 requested a review from TysonMN July 26, 2022 21:05
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 8dfa361 to bda70bb Compare August 6, 2022 20:44
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from bda70bb to 24a541d Compare August 6, 2022 22:11
@marner2 marner2 force-pushed the feature/add_vm_type_information branch 2 times, most recently from e9acbae to bbdec69 Compare August 17, 2022 12:52
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from bbdec69 to ec21987 Compare August 30, 2022 20:06
src/Elmish.WPF/BindingData.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingData.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingData.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingData.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/Binding.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/Binding.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/Merge.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingData.fs Outdated Show resolved Hide resolved
src/Elmish.WPF/BindingVmHelpers.fs Outdated Show resolved Hide resolved
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from e174e0f to 6565689 Compare September 6, 2022 20:30
@marner2 marner2 changed the title Add vm type information to BindingData and VmBinding Add vm type information to BindingData Sep 6, 2022
@marner2 marner2 changed the title Add vm type information to BindingData Add output type information to BindingData Sep 6, 2022
@marner2 marner2 force-pushed the feature/add_vm_type_information branch from 6565689 to 2505953 Compare September 6, 2022 23:13
@TysonMN TysonMN merged commit cb10cc5 into elmish:master Sep 7, 2022
@marner2 marner2 deleted the feature/add_vm_type_information branch September 7, 2022 01:47
@TysonMN
Copy link
Member

TysonMN commented Sep 8, 2022

For each comment that had a pin put in it, can you add a comment on the new PR that is adding that code, link back to the pinned comment here, quote what I asked, and (if you responded here) quote your response?

@LyndonGingerich
Copy link
Contributor

For each comment that had a pin put in it, can you add a comment on the new PR that is adding that code, link back to the pinned comment here, quote what I asked, and (if you responded here) quote your response?

Done for every comment explicitly referenced as pinned for a future PR.

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