search

elmish / Elmish.WPF

Static WPF views for elmish programs
Other
421 stars 68 forks source link

All @rfreytag contributions removed while retaining all merged commits by others up to 20230523 #569

Closed rfreytag closed 1 year ago

rfreytag commented 1 year ago

All @rfreytag contributions removed while retaining all merged commits by others up to 20230523

TysonMN commented 1 year ago

I see two commits in master by you. Is the intention of this PR to revert both of those commits?

rfreytag commented 1 year ago

@TysonMN As I understand it that is what you wanted. Right?

All my changes except 'fully-specified parameter lists' received equivocal reactions. However keeping 'fully-specified parameter lists' runs into the issue of whether or when to use unit in the parameter lists. So I undid all my changes letting you long-time committers decide how to proceed.

TysonMN commented 1 year ago

Ok. I created a PR #570 instead of this one. The proper way to undo changes is to revert a commit in git. Since this PR of your didn't do that, I was confused about that you intention was with this PR. My PR reverted the merge commit of your previous PR. There were a couple small conflicts and I manually resolved.

One reason some objectionable changes made it past PR review in that your PR was large and made many different changes. I recommend readings 10 tips for better Pull Requests by Mark Seemann, especially the first two items. As a first-order approximation, each PR review gets the same amount of attention. Thus, if a PR is small, then each changed line gets a lot of attention, but if the PR is large, then each changed line gets very little attention, which causes things to get missed.

Once you have split up a large PR into smaller ones, the next thing to consider is the order in which you will submit them. The correct order is from least contentious to most contentious. That can be easier said than done because it is not always clear what the reviewer will consider contentious. As you gain more programming experience, you will get a general sense of what is typically considered contentious. As you repeatedly work with specific people, you will learn what they consider to be contentious.

From memory, I can think of four different changes your previous PR made:

  1. Adding type annotations
  2. Removing blank lines
  3. Renaming modules
  4. Renaming functions

You should make separate PRs for each of these changes.

I tried to write to order them from what I think is generally considered least to most contentious. However, I don't I will approve any of your changes to remove blank lines. An F# convention is that the number of blank indicates how related the functions or modules are (source):

Extra blank lines may be used (sparingly) to separate groups of related functions. Blank lines may be omitted between a bunch of related one-liners (for example, a set of dummy implementations). Use blank lines in functions, sparingly, to indicate logical sections.