GeezerVison

I should use Measure-Object in the test but instead used Measure-Command, which pops for mandatory parameter input and thus caused the build to hang. It's fixed now.

Ilya (@iSazonov) I have addressed your comments. Please take another look. Thanks!

Jason Shirk (@lzybkr) Aditya Patwardhan (@adityapatwardhan) We have new commits - please take another look.

Jason Shirk (@lzybkr) Can you approve?

Ilya (@iSazonov) Jason probably doesn't have time to come back and approve it again, so it's up to the maintainer's judgment to see if it's necessary for reviewers who had approved to approve again. There are some guidelines you can use, for example:

1. are the changes after the previous approval fundamental or just touch-ups
2. are the changes to address comments left by the reviewer who previously approved

Certainly, you will get your own guidelines as you become more experienced in this role.

Dongbo Wang (@daxian-dbw) Thank you for the helpful comment!

In this case, it's more of a courtesy - I have to show respect and ask him after I formally dismissed his approval.

Ilya (@iSazonov) Another related comment. I usually don't dismiss a reviewer's approval unless the PR needs some fundamental changes (e.g. design flaw, logic changes and etc.). If the following-up changes are touch-ups or minor fixes (like typo, a null condition check, test fix-up), I think it's fine to keep the previous approval, and once the minor issues are addressed, you can approve the PR and merge it.

Dongbo Wang (@daxian-dbw) I wonder why CI AppVeyor failed?

I have investigated the failure, it actually a bug in Binder, more specifically at here, the restriction generated in the call to this.DeferForPSObject doesn't respect the fact that the baseobject needs to be a COM object.

A repro would be:

$shell = New-Object -ComObject "Shell.Application"
$folder = $shell.Namespace("F:\tmp")
#$item = $folder.Items().Item(0)
$item = $folder.Items() | select -Last 1
$item.Name


$members = new-object System.Collections.ObjectModel.Collection[System.Management.Automation.PSMemberInfo]
$n=new-object Management.Automation.PSNoteProperty a,1
$members.Add($n)
$r=add-member -InputObject a -MemberType MemberSet -Name Name -Value $members -passthru
$r.Name.a

The $r.Name.a is supposed to return 1. If you run the lower part first then upper part, then all work fine.
This happens when a COM object is wrapped into a PSObject, which is the case when the COM object is returned from a pipeline, like the above repro code.

I cannot open an issue until this PR is merged because the repro requires enumerating a COM collection.
I have updated the test to not access Name member, so it would be good now. When I resolve the bug, I will update the COM basic test to keep accessing the Name member.

Dongbo Wang (@daxian-dbw) Maybe made two commits for code and tests before rebase and merge?

Ilya (@iSazonov) Please just choose 'Squash and merge'. It's a simple fix and there is no need to keep history commits.

A follow-up of the binder bug:
The issue repros without COM enumeration. #4607 opened to track it.

Dongbo Wang (@daxian-dbw) Merged.
Great work! Thanks!

Ilya (@iSazonov) Thank you for the review and good work as a maintainer 😄

Here are some of my experiences to choose among Squash and merge, Rebase and merge and Create a merge commit:

1. number of meaningful commits (commits that are part of the PR work, or commits to address major comments that require non-trivial design/logic changes) is one thing to consider
2. the complexity of the PR work is also something to consider.

• Some PR may involve many commits, but the fix or feature work is not complex, in those cases, just Squash and merge.
• Some PR may have clean history commits, but again, the fix/work is simple, in those cases, Squash and merge
• A PR may be complex work, and the history is not clean -- complex work usually means a lot major changes during the review process -- in such case, choose create a merge commit to preserve the history even though it's not clean. Example: Native pipe #2450
• A PR is non-trivial and the history is clean, then it's worthy to keep the history. In this case, consider using rebase and merge to keep the main branch history clean if the number of commits is not too many (~5/6). Otherwise, use create a merge commit.
• A PR may contain more than 1 fix, in this case we usually should ask the author to break it into two PRs.

As you can see from the main branch history, most of time we use (and prefer to) squash and merge.

As for what is a simple/complex/non-trivial fix/work, it's hard to describe but you will have your judgement call as you gain more expeirences.

Dongbo Wang (@daxian-dbw) Thanks for sharing your experience!