Skip to content

Conversation

@vexx32
Copy link
Collaborator

@vexx32 vexx32 commented Sep 3, 2019

PR Summary

  1. Replaces all WriteInformation() calls with WriteVerbose() Removes all verbose and progress output. We will revisit verbose output in a later PR.
  2. Refactors ping implementation to create only a single Ping object which is reused for all Send() calls.
  3. Implements IDisposable for Test-Connection so that it can dispose of the Ping object when it is no longer needed.

PR Context

Part 2 of #10044, refactor of Test-Connection cmdlet.
Fixes #6768

PR Checklist

@vexx32 vexx32 changed the title Test-Connection - Move informational output to Verbose stream Test-Connection - Move informational output to Verbose stream, consolidate Ping usage Sep 3, 2019
@iSazonov
Copy link
Collaborator

iSazonov commented Sep 5, 2019

Replaces all WriteInformation() calls with WriteVerbose()

Intention was to write to Host. PowerShell implements this over information stream.
A conclusion was to be close to Windows PowerShell so I see no reason to write to verbose stream in the same way. I believe we need to remove this.

@vexx32
Copy link
Collaborator Author

vexx32 commented Sep 5, 2019

That is true...

I was thinking of a use case where -Quiet may be used in the code, but during development the -Verbose parameter could be added to show the otherwise hidden/ignored information.

Though I suppose the verbose output probably should just be something along the lines of: VERBOSE: Testing connection to <target>?

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 5, 2019

Yes, verbose output should has another form and probably be in other places because it serves other purposes. We could implement this later if this will be needed.

@vexx32
Copy link
Collaborator Author

vexx32 commented Sep 5, 2019

I'll do a revision of the verbose output today, keeping it pretty simple. We can expand on it more at a later date if we need more. :)

@iSazonov
Copy link
Collaborator

iSazonov commented Sep 5, 2019

You could simply remove the extra output in the PR and add verbose in follow PR to speed up code review.

@vexx32
Copy link
Collaborator Author

vexx32 commented Sep 5, 2019

@iSazonov would it be better to also remove progress bars in this PR or following PR?

EDIT: It seems that change is a bit simpler at least from looking at the diffs, since we can just remove entirely the methods that handle the information/progress data. If you'd rather I leave the progress in for now, just ping me and I'll make the change. 🙂

:burn: Remove unused resource strings
@vexx32 vexx32 changed the title Test-Connection - Move informational output to Verbose stream, consolidate Ping usage Test-Connection - Remove informational output, consolidate Ping usage Sep 5, 2019
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Sep 6, 2019
@iSazonov iSazonov added this to the 7.0.0-preview.4 milestone Sep 6, 2019
@vexx32
Copy link
Collaborator Author

vexx32 commented Sep 26, 2019

@TravisEz13 is there anything more you guys need from me on this one? I have a couple more PRs lined up and ready for this cmdlet that I'd love to get in for PSv7 to more fully cover the scope of the approved RFC. 💖

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 3, 2019

@SteveL-MSFT @TravisEz13 I know you guys are pretty busy at this point in the release cycle, but can't hurt to ask I hope:

Can I ask to get this one merged in and move forward with implementing the remainder of the changes already approved in the RFC for v7 timeframe? I have the code for it all written (at least one or maybe two more PR's worth) and the RFC is already approved, so I would like to have all the Test-Connection changes I plan on making merged in for v7 GA if at all possible.

Thank you! You guys are awesome! 😊 💖

@iSazonov iSazonov self-assigned this Oct 3, 2019
@TravisEz13
Copy link
Member

Did the output of the Cmdlet change in this PR?

@vexx32
Copy link
Collaborator Author

vexx32 commented Oct 3, 2019

@TravisEz13 The success output of this cmdlet has not changed. That is what I plan to change in a follow-up PR. 🙂

Or, if you'd prefer, I can merge the changes from that branch into here directly, but that does make the diffs unfortunately difficult to work through, which is why I subdivided my PRs on this one. 🙂

@TravisEz13
Copy link
Member

No... I prefer it like this. I'm just trying to clarify.

@TravisEz13 TravisEz13 merged commit 1d77986 into PowerShell:master Oct 3, 2019
@vexx32 vexx32 deleted the TestConnection/Verbose branch October 3, 2019 19:40
@ghost
Copy link

ghost commented Oct 23, 2019

🎉v7.0.0-preview.5 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test-Connection cmdlet displaying unwanted data.

4 participants