Check type of actual argument using consistent helper method

[jnm2]

No description provided.

[ChrisMaddock]

Nice finds. 🙂

[oznetmaster]

I am confused as to how this was merged/committed with only one reviewer?

Also, this shows up in github desktop as not being a commited merge pull request.

[mikkelbu]

@oznetmaster We only require two members to review for non-member PRs. For PRs from members we only require one other member to review (I cannot find these exact rules in the governance repo, but I was told this in #2290 (comment)).

Wrt. it not being a "commited merge pull request", then I think that this PR was merged using "Rebase and merge" option, if I understand correctly what you mean by "not being a commited merge pull request".

[oznetmaster]

@mikkelbu That is news to me. The rules must have changed at some point in time.

I filter all commits to only show merge pull requests, so if they are not merge pull requests, I never see them :( It used to be that every commit would appear twice, once as a commit, and once as part of a merge pull request, which was when it was really "done".

This has never been an issue in the past. Something must have changed here as well.

[mikkelbu]

I don't know when the rules was changed, as I have only known the current rules.

Regarding the merging of pull requests then github/git provides different options (see e.g. https://help.github.com/en/articles/merging-a-pull-request). We have so far in NUnit almost solely used the default which creates a merge commit on the master branch. The other options "moves" the commit(s) to the master branch without a merge commit.

I don't know how - and where - you do the filtering, but is it possible for you to do filtering on the branch name? If so, then you could filter for commits on master, and you would capture all changes no matter how they were merged to master.

[CharliePoole]

When I set up the NUnit 3 project I created the policy that it takes two commiters to approve a merge. In the case of a PR created by a commiter, that meant one review. A PR from someone outside the team needed two reviews. That has never changed.

[jnm2]

I really don't care, but I'm partial to having a merge commit with two parents (uses git's --no-ff option). It makes pretty tree graphs and captures the branch name in the commit message by default.

But either way, I like the 'first-parent' filter concept that Azure DevOps Server offers when displaying the commit graph. It ends up showing only merge commits, but it seems like it would also follow direct commits on the master branch such as rebase merge commits. Maybe GitHub Desktop has something similar. When it's possible, I like making tooling work around us versus following a practice just to make tooling happy.