robot-name: Fix hlint-1.9.38 newtype warning

[rbasso]

Patch example solution to fix the warning:

./exercises/robot-name/examples/success-standard/src/Robot.hs:7:1: Suggestion: Use newtype instead of data
Found:
  data Robot = Robot{robotNameVar :: MVar String}
Why not:
  newtype Robot = Robot{robotNameVar :: MVar String}
Note: decreases laziness

1 hint (5 ignored)

The nightly snapshot currently fails the tests in Travis-CI because of that warning.

[petertseng]

I seem to recall the "Update branch" button creates a merge commit from master into this branch, rather than rebasing, so it seems a bit inconvenient. You can try it if you want

(I only mention that because of the "Require branches to be up to date before merging" setting)

I will do some testing on this now.

[rbasso]

I guess that Katrina decided to enable "Require branches to be up to date before merging" too. This one I was not expecting, but seems great.

Edit: agreed! inconvenient.

[petertseng]

Tested at https://github.com/petertseng/mergetest/pull/2 and https://github.com/petertseng/mergetest/pull/3 - a merge commit is created.

If using Squash and merge, it's fine, it disappears.

If using plain merge, the merge commit stays such as https://github.com/petertseng/mergetest/commit/7eae77d99fbac3a8af05b2f99e935dc8d36ddf81, and I consider this unclean history rather than rebasing

So, this effectively means if multiple PRs are in flight, we have to use Squash and Merge on all-but-one (all but the first one) of them. I don't care about this if the PR consists of only one commit, but it will be slightly inconvenient if there is a PR with multiple commits (that should stay as multiple commits when merged): I would have to either wait for the contributor to rebase the pull request, or rebase it myself (and I'm unlikely to want to go to that much trouble)

[rbasso]

In that case, do you think we should ask Katrina to disable "Require branches to be up to date before merging"?

Do you have anything against the other protections? (approval + pass checks)

[rbasso]

I'll make one more test....hang on.

[petertseng]

In that case, do you think we should ask Katrina to disable "Require branches to be up to date before merging"?

My personal experience is that in the past, it has slowed my team down unreasonably.

The additional situation that "Require branches to be up to date before merging" protects against is if we have branches A and B that both pass checks individually, but fail checks together.

I did not personally get enough value out of this protection to justify the additional work (either: rebase every PR, or have ugly merge commits, or be able to only have one commit per PR because of having to squash).

So I do support unchecking only that particular box, unless we can find a way to make it work.

Do you have anything against the other checks protections? (approval + pass checks)

Those are OK with me! They have slowed us down... reasonably!

[rbasso]

Ok. One last question before asking Katrina to change that.

There is one thing I may be missing. Have you tested with the new "Rebase and Merge"?
Would that work? What would be the problem?

[petertseng]

I will delete my test repo soon, so I will reproduce what I've got in writing:

I have a travis.yml file whose script is: if [ "$(ls -1 files | wc -l)" = "2" ]; then exit 1; fi - very simple, fail the check if there are TWO files.

I make two PRs, one to add files/a and one to add files/b. They will both pass individually but fail if together.

I merge the first PR. The check for the second PR still passes, since it has not updated. I can, however, rerun it, and then it fails as expected (since Travis tests the result of merging into master, not the PR as it branched off of master)

---

Have you tested with the new "Rebase and Merge"

All my observations were made before this feature existed. Please hold while I test.

[petertseng]

Have you tested with the new "Rebase and Merge"

My test procedure:

• Create PR with two commits, but is out of date with current master.
• Click "Update branch"
• Choose "Rebase and merge"

Result: The two commits are put directly on master, and the merge commit "Merge branch 'master' into branchname" is not present on master (good!).

This result is acceptable.

The small slight disadvantage is that it becomes harder to see when a PR has been merged, since we do not get to see the commit "Merge pull request #X from contributorname/branchname". The only way we will be able to tell what PR a particular commit came from would be to look at the commit on Github web interface e.g. https://github.com/petertseng/mergetest/commit/b4824eca31c5b481f633cb08f6c6b562ec8ec0ff and then the PR is shown there. I don't believe the information is available without an internet connection e.g. if I just use git log locally

If we are willing to accept that small disadvantage in exchange for the additional protection, I think I am in, because I don't think I often make use of the information gained from "Merge pull request #X" commits.

So I have updated from "strongly prefer unchecking that checkbox" to only "very very slightly prefer unchecking that checkbox" - it would be acceptable to leave it.

[rbasso]

So I propose that we, temporarily, leave it the way it is. This way we can get a little more experience on the pros and cons of this approach, and I think it will be a good experience.

When you decide that we had enough testing, we'll revert it.

So...can you update and "rebase merge" this one, so that we can see how it looks when someone else does it? 😄

[petertseng]

So...can you update and "rebase merge" this one, so that we can see how it looks when someone else does it?

Coming right up (I imagine Github will write the commits such that they will be authored by you, and committed by me).