Adding fully specified parameter lists to NewWindow.Core and FileDialogsCmdMsg.Core

[rfreytag]

I found Sample/NewWindow.Core/App.fs hard to read. I have a version in my fork, branch: Adding_fully-specified_parameter_lists that I find more readable with the following changes:

• Every function has fully-specified parameter lists and return values. Readability is improved by not having to hover over function names to see what the function manipulates.
• module App, module Window1 and module Window2 have been renamed module App_Window1 and module App_Window2 so their usage in App.bindings is not confused with the actual references to Window2 in App.bindings.
• Clarified module App, module Window1, and module Window2: get, set, and map so they specify the type on which they operate for readability. They are now respectively:
  • module App, module Window1: get_Window1State1, set_Window1State1, and map_Window1State1 and
  • module App, module Window2: get_Window2, set_Window2, map_Window2.
• Replaced all usage of function with explicit match with to support with the explicit parameter lists.

I added fully-specified parameter lists to the remaining Sample/NewWindow.Core F# files while I was at it. And then for good measure I did the same to FileDialogsCmdMsg.Core.

Doing this helped me understand what was going on. I hope it will help others adopt Elmish.WPF. I will issue a pull request if you like where I'm going.

[marner2]

@LyndonGingerich @TysonMN do you think we should we should do something similar with the rest of the examples?

[rfreytag]

@marner2 Yes, I do think this edit for readability would help with the other examples. I am adding these parameter lists and return values to Capabilities.Core now. Could you merge this request so I can branch from 'master' again?

[marner2]

Ok sounds good. Please avoid any samples touched by #551 for now to avoid conflicts (until it gets merged in).

[rfreytag]

FileDialogsCmdMsg.Core changes were merged in with the commit; sorry. But I can stay away from:

• FileDialogs.Core
• SubModel.Core
• SubModelStatic.Core

I plan on next working on Capabilities.Core and perhaps merging in FileDialogsCmdMsg.Core.

[TysonMN]

@rfreytag, since this PR was already merged, can you create another PR that address my concerns?

[LyndonGingerich]

I don't think this change was thought through with a lot of nuance. I see lines such as let init : string = "", which doesn't need a type annotation.

Unless I misunderstand, the samples attempt to communicate simply and clearly with those who are used to idiomatic F#. We do not wish to require readers to learn a new set of idioms specific to Elmish.WPF; many probably need only one or two samples to browse. Since the reader is already trying to learn a new framework, he should not have to learn a new way of writing F# at the same time. For this reason, I propose that we do not introduce new idioms that F# developers cannot immediately understand and effortlessly remember.

Changes this PR makes:

• Separating sections of the code with one newline rather than two. Newline style varies by codebase; so long as we use newlines consistently and separate sections of the code, I don't think we should worry about their numbers. This change should not have been made.
• Adding type annotations. Type annotations are situationally useful, but they also add more text to read. We should not annotate the types of identifiers which themselves specify their types (e.g. cmdMsg: CmdMsg). This changed should have been considered individually. I don't know when type annotations help others, but I probably would not have added most of these myself. We have better ways of increasing clarity.
• Eta-expansion of function to match. I have no strong opinions on this change, but I will suggest that type unclarity on a function might be better resolved by qualifying the discriminated union cases (and probably adding RequireQualifiedAccess to the DU).

On the other hand, we do use idioms in this sample that most F# developers probably wouldn't recognize:

• init for initial values. But this is an Elmish idiom, so I think we should keep it.
• Model for the top-level record type. Developers will probably understand this.
• msg to mean "message". This is a simple abbreviation and probably clear.
• m to mean "model". I think this abbreviation is likely unclear. I move to expand it to model. Where the type in question is not called model, we should rename it to reflect the type in question.
• flip. flip decreases readability by increasing semantic code complexity. Let's replace it with lambdas.
• AutoOpen.map. This function is difficult to read and comprehend, but replacing it might increase the code length significantly. Could we at least rename it to the more familiar (and less overloaded) lens?
• InOut. This DU is simple and I don't see how we could replace it with anything simpler anyway. We could use Choice, but InOut has more meaning. (If we decide that the effort of learning the in-message/out-message concept outweighs the organization, though, then we should replace InOut with Choice.)
• cata. I know what cata means, and Mark Seeman knows what cata means, but I don't think most developers would recognize it. Dictionary.com defines it as "'down', 'against', 'back'", which would not make its meaning clear even to one familiar with the prefix. Let's inline it. match is quickly and easily readable.
• get/set/map. I see most people using lambdas. Let's replace get and set with lambdas and thus reduce mental overhead. We can keep map where it's useful, but let's rename it to lens.
• mapOutMsg/mapInOutMsg: These don't need to be so far away from their call sites. They are trivially short and used only once each. Let's inline them.

[LyndonGingerich]

LyndonGingerich TysonMN do you think we should we should do something similar with the rest of the examples?

@marner2 Above post intends to answer your question.

[marner2]

Yeah I think those suggestions in the last half of your comment make sense @LyndonGingerich, and we definitely should be more considered and deliberate with future sweeping updates of the samples.

I was ok with the update as-is because it's always valuable to get empirical feedback when someone unfamiliar with the project details their experience (and it was just a single file that was updated).

As far as explicitly specified types taking longer to read, while that may be true of production code, samples have a slightly different target. Which is again why I'm ok with liberal eta-expansion (especially of function to match) showing up in the samples. It just helps guide people who unfamiliar with the idioms. The other thing about these sample projects is that they are intentionally short, and generally in very small implementations you should err more on the side of verbosity. When you have a very large codebase where things get repeated a bunch, that's where the shortcuts start to kick in and pay off.

[TysonMN]

Adding more type annotations is typically fine. There were some in this PR that don't improve readability, and those can be removed.

[LyndonGingerich]

Here's my first effort at a low-complexity version of the sample program. I'm not making a PR because I'm intending to diff on the original, not the current version with this PR merged.

https://github.com/LyndonGingerich/Elmish.WPF/compare/070d311428042676f7f2b92fdd5f40c504c6e4d1...LyndonGingerich:Elmish.WPF:lyndon_suggested_simplifications_05-19-2023

@rfreytag , would these changes have made the code easier to read?

[rfreytag]

@TysonMN I will remove the changes to which you objected and reissue the PR. In my defense, I did offer my branch for review before issuing a pull request.

Glad to learn from this discussion.

[TysonMN]

Yes, no worries. You didn't do anything wrong. It is easy to change code (back).

[rfreytag]

As you have time would you review my fork, branch Adding_fully-specified_parameter_lists latest commit (6de8aae7)?

The commit note reads... "

• Reverted get, set, and map per @TysonMN preference.
• Reverted Window1.init, Window2.bindings per @LyndonGingerich preference.
• Moved init, update, and bindings out of App to allow App.Window1 and App.Window2 for readability in both bindings and update.
• Program.fs now references AppModule.init, AppModule.bindings and AppModule.update.
• Removed/replaced two usages of unit per @LyndonGingerich but fully-specified parameters seemed to require the remaining unit references.
• Reverted all line-spacing edits (i.e. in NewWindow.Core and FileDialogsCmdMsg.Core). "

If this doesn't suit I accept it might be best if I stay in my fork.

I'll issue the PR if you folks are satisfied.

[rfreytag]

@LyndonGingerich I have been rushing to revise/undo my prior PR. Will read your thoughts with interest as time allows.

[TysonMN]

Please first create a PR that only addresses the concerns I raised on this PR. After that is merged, we can look at your other improvement suggestions.