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

Rename onewayseqlazy types #471

Closed

Conversation

marner2
Copy link
Collaborator

@marner2 marner2 commented Jun 2, 2022

Ready to merge

@marner2 marner2 force-pushed the feature/rename_onewayseqlazy_types branch from 53d181a to 1ef1640 Compare June 2, 2022 21:29
@marner2 marner2 force-pushed the feature/rename_onewayseqlazy_types branch from 1ef1640 to 5ad4c17 Compare June 4, 2022 15:33
@marner2
Copy link
Collaborator Author

marner2 commented Jun 4, 2022

I tried to name them similar to what the equivalents are in the other types. It's a little different, but still accurate enough IMO.

@marner2 marner2 force-pushed the feature/rename_onewayseqlazy_types branch 2 times, most recently from 3a71d91 to d8a4a34 Compare June 7, 2022 03:30
@marner2
Copy link
Collaborator Author

marner2 commented Jun 7, 2022

@TysonMN this is ready for merging

@marner2
Copy link
Collaborator Author

marner2 commented Jun 7, 2022

If you use the merge commit method, I believe the next PR #470 will be able to update from the Github UI.

@TysonMN
Copy link
Member

TysonMN commented Jun 9, 2022

I don't see how this is making things more consistent. It seems less consistent to me.

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.

I have a question

@marner2 marner2 force-pushed the feature/rename_onewayseqlazy_types branch from d8a4a34 to 684b852 Compare June 9, 2022 17:10
@marner2 marner2 force-pushed the feature/rename_onewayseqlazy_types branch from 684b852 to 8cade63 Compare June 9, 2022 17:17
@marner2

This comment was marked as outdated.

@TysonMN
Copy link
Member

TysonMN commented Jun 10, 2022

Yes! I can rerun a build from my phone using GitHub's app.

@@ -410,18 +410,18 @@ module internal BindingData =
module OneWaySeqLazy =

let mapMinorTypes
Copy link
Member

Choose a reason for hiding this comment

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

I think this method is clearer with the shorter names. For example, the function inMapA is passed arguments a1 and a2.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's to match it with the other names that are used for similar bindings. None of the other bindings use 'a and 'b. They all use 'bindingModel and 'bindingViewModel. See line 616 in this file for SubModelSeqUnkeyed and line 665 for SubModelSeqKeyed.

Copy link
Collaborator Author

@marner2 marner2 Jun 10, 2022

Choose a reason for hiding this comment

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

Also SubModelWin uses the same names, SubModel as well but its function is in another file (I move it here in another PR).

@TysonMN
Copy link
Member

TysonMN commented Jun 10, 2022

Let's do PR #477 first

@marner2 marner2 marked this pull request as draft June 10, 2022 17:30
@marner2
Copy link
Collaborator Author

marner2 commented Jun 10, 2022

@TysonMN I'm removing this PR from my chain. Do you need #477 done before the others?

@marner2 marner2 closed this Jun 10, 2022
@TysonMN
Copy link
Member

TysonMN commented Jun 10, 2022

I think it would be best to merge PR #477 first since it mostly deletes code.

Do you know if there are similar trivial simplifications? (I have wanted to make that change for a long time, but I didn't realize that all my previous work made it so easy.)

@marner2
Copy link
Collaborator Author

marner2 commented Jun 10, 2022

@TysonMN not really. I have a PR that much better sorts the BindingData from Binding, and that results in a lot of simplifications.

@marner2
Copy link
Collaborator Author

marner2 commented Jun 10, 2022

@TysonMN also I closed this PR and removed the commits since it wasn't making sense to do it.

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