Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test-Connection cmdlet displaying unwanted data. #6768

Closed
NJ-Dude opened this issue Apr 28, 2018 · 63 comments · Fixed by #10478

Comments

@NJ-Dude
Copy link

@NJ-Dude NJ-Dude commented Apr 28, 2018

Steps to reproduce

The Test-Connection cmdlet is displaying unwanted data as part of the result.

Code:

Test-Connection www.microsoft.com -Count 1 -Quiet

Expected behavior

It should display just the word: True

Actual behavior

Pinging www.microsoft.com [23.204.153.19] with 32 bytes of data:
Reply from 23.204.153.19: bytes=32 time=17ms TTL=58
Ping complete.
True

Environment data

> $PSVersionTable

Name                           Value
----                           -----
PSVersion                      6.1.0-preview.2
PSEdition                      Core
GitCommitId                    v6.1.0-preview.2
OS                             Microsoft Windows 10.0.16299
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0
@BrucePay

This comment has been minimized.

Copy link
Collaborator

@BrucePay BrucePay commented Apr 30, 2018

This works properly in 5.1 but not in 6 so I've reclassified the issue as a bug.

@GeeLaw

This comment has been minimized.

Copy link

@GeeLaw GeeLaw commented May 1, 2018

The reason is that current code calls WriteInformation (blindly?).

See line 751 of TestConnectionCommand.cs, also line 775 and line 783.

Temporary workaround to stop the information from being displayed to the host is to use InformationAction common parameter. Example:

Test-Connection www.microsoft.com -Count 1 -Quiet -InformationAction Continue

From scripting perspective, this wouldn't be an issue, as the textual information is never written to the pipeline, and no, the textual output is not part of result data, which are defined as things sent to the pipeline. Also, Quiet switch is defined to return simpler results (int or bool, instead of record objects). I have to admit that one might not expect InformationRecord with Quiet. However, knowing the reason, I say we'd better keep InformationAction and Quiet decoupled.

In PowerShell 5.1, Test-Connection does not seem to call WriteInformation at all. By the way, the default value for $InformationPreference in PowerShell 5.1 and PowerShell Core 6.0.2 is SilentlyContinue. The author of the issue might have a different effective value when he reproduced the issue. (Perhaps PS 6.1 Core changed the default value for $InformationPreference? I'm not sure.)

If PS 6.1 Core had $InformationPreference defaulted to SilentlyContinue, the textual information wouldn't be there unless the user explicitly asks for it.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented May 19, 2018

I need more feedback.
The problem is that in the script mode and in interactive mode it should work in different ways. In interactive mode the user will probably prefer to see the progress (bar) as it happens with the ping.exe command. This applies to other parameters too.

@mklement0 If you have time then your help would be useful.

@mklement0

This comment has been minimized.

Copy link
Contributor

@mklement0 mklement0 commented May 19, 2018

@GeeLaw:

Good find, but it is -InformationAction Ignore that is needed in this case to work around the bug.

$InformationActionPreference still defaults to SilentlyContinue, and that shouldn't change.

The bug is that the WriteInformation() calls you link to mistakenly use the PSHOST tag, which effectively bypasses the $InformationActionPreference value and also -InformationAction SilentlyContinue (but, as stated, -InformationAction Ignore is effective in suppressing the output).

What said WriteInformation() calls do is effectively what Write-Host does in order to unconditionally display its output (by design).

@iSazonov: I haven't really looked at the behavior of other cmdlets with progress bars, but with -Quiet I would not expect a progress bar, even when calling interactively.

@GeeLaw

This comment has been minimized.

Copy link

@GeeLaw GeeLaw commented May 20, 2018

@mklement0 Thanks for the reply and the correction on PSHostTag. It seems to me that InformationPreference (InformationAction) won't accept Ignore? This is true in 5.1 and 6.0.2. Perhaps in 6.1 it's changed. (Is InformationActionPreference a new alias for InformationPreference?)

Also, my first comment was mistaken, for that it sets InformationAction to Continue. The correct workaround is to discard stream 6 (information stream) by redirecting it to $null, i.e.,

Test-Connection www.microsoft.com -Count 1 -Quiet 6> $null

Check the correctness by the following:

Write-Host 'Hello, world' 6> $null

It should write nothing to the host.

@mklement0

This comment has been minimized.

Copy link
Contributor

@mklement0 mklement0 commented May 20, 2018

@GeeLaw:

It seems to me that InformationPreference (InformationAction) won't accept Ignore?

Yes, the preference variable doesn't accept Ignore, but the common parameter does.

That is, you can suppress the information stream (number 6) for a given invocation, but not categorically for the whole scope - by design.

Therefore, the following two statements are equivalent:

Test-Connection www.microsoft.com -Count 1 -Quiet 6> $null
Test-Connection www.microsoft.com -Count 1 -Quiet -InformationAction Ignore

In other words: both 6> $null and -InformationAction Ignore are effective workarounds.

Is InformationActionPreference a new alias for InformationPreference?

No, the name has always been $InformationPreference, following the pattern of $VerbosePreference, $WarningPreference, and $DebugPreference.

From what I can tell it's only the -ErrorAction / $ErrorActionPreference pair where the preference variable name retains the Action part.


As an aside regarding the prohibition of Ignore as the value of action preference variables:

  • There's a fundamental design problem with this restriction, because automatically defined local preference variables are used to propagate common-parameter values inside advanced functions - see #1759

  • The restriction is not enforced at assignment time, which means you won't see the problem until the preference is (possibly implicitly) applied the next time - see #4348

    • More generally, in any scope but the global one, no validation is performed at all; compare $ErrorActionPreference = 'bogus' to & { $ErrorActionPreference = 'bogus' } - see
      #3483.
@GeeLaw

This comment has been minimized.

Copy link

@GeeLaw GeeLaw commented May 20, 2018

@mklement0 I was asking about the alias because you mentioned it as InformationActionPreference.

To my best knowledge, -XxxAction (and -Verbose and -Debug) simply sets the corresponding preference variable inside the invoked cmdlet. As documented in about_ help topics, specifically we have the following:

The value of the -InformationAction parameter, if used, overrides the current value of the $InformationPreference variable.

Within the command or script in which it is used, the InformationAction common parameter overrides the value of the $InformationPreference preference variable, which by default is set to SilentlyContinue.

I interpret this as setting a local preference variable, as can be validated by the following demonstration:

function test-func { [cmdletbinding()]param() process { $InformationPreference } }
test-func # gives SilentlyContinue
test-func -InformationAction Continue # gives Continue
test-func -InformationAction Ignore # gives Ignore

In 5.1 and 6.0.2, InformationAction does not accept Ignore, as can be demonstrated by the following snippet:

function test-func { [cmdletbinding()]param() process { write-information 'writing' } }
test-func -InformationAction Ignore

produces

Write-Information : The value Ignore is not supported for an ActionPreference variable. The provided value should be used only as a value for a preference
parameter, and has been replaced by the default value. For more information, see the Help topic, "about_Preference_Variables."
At line:1 char:57
+ ...  { [cmdletbinding()]param() process { Write-Information 'writing' } }
+                                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Write-Information], NotSupportedException
    + FullyQualifiedErrorId : System.NotSupportedException,Microsoft.PowerShell.Commands.WriteInformationCommand

I wonder if that changed in 6.1.


There's another interesting thing: $VerbosePreference and $DebugPreference actually accepts a wider range than that can be set by the corresponding common parameter. E.g., it is possible to set $VerbosePreference to Inquire by explicit assignment, but not possible by using -Verbose switch because, after all, it is a switch and maps $True/$False to Continue/SilentlyContinue.

As far as I know, except for those two switches, the preference-related common parameters map exactly to the corresponding preference variables in the local scope.

@GeeLaw

This comment has been minimized.

Copy link

@GeeLaw GeeLaw commented May 20, 2018

Another round of playing around shows the following code works:

Write-Information 'writing' -InformationAction Ignore

But that would be cumbersome to developers, as we have to guard Write-Information with the check $InformationAction -eq 'Ignore', and avoid calling it in the first place, e.g.,

Function Test-Inf1
{
    [CmdletBinding()] Param ( )
    Process { Write-Information 'Hello, world!' }
}
Function Test-Inf2
{
    [CmdletBinding()] Param ( )
    Process { If ($InformationPreference -ne 'Ignore') { Write-Information 'Hello, world!' } }
}
Test-Inf1 -InformationAction Ignore # writes an error
Test-Inf2 -InformationAction Ignore # okay

It seems that the same applies to authors writing cmdlets with C#.

@mklement0

This comment has been minimized.

Copy link
Contributor

@mklement0 mklement0 commented May 20, 2018

@GeeLaw

I was asking about the alias because you mentioned it as InformationActionPreference.

Oops! Sorry - hadn't noticed my own typo.

simply sets the corresponding preference variable inside the invoked cmdlet.

Yes, and the rest of what you demonstrate is the subject of the aforementioned #1759.

In short: The design flaw is that disallowing Ignore as a preference-variable value clashes with using automatically set local preference-variable instances in order to propagate common-parameter values.

More specifically:

In 5.1 and 6.0.2, InformationAction does not accept Ignore, as can be demonstrated by the following snippet:

Since it matters with respect to resolving the problem, let me point out that -InformationAction does accept Ignore, and it's only said design flaw that causes the problem if and when the preference is applied in the context of advanced functions, which in your case is the Write-Information call inside the function .
This problem persists as of PowerShell Core v6.1.0-preview.2.

Compiled cmdlets, by contrast, do not have this problem, which is why Write-Host foo -InformationAction Ignore works as intended, for instance, as you've since discovered.

@mklement0

This comment has been minimized.

Copy link
Contributor

@mklement0 mklement0 commented May 20, 2018

To summarize, we've been discussing two unrelated problems here:

  • The design flaw with respect to Ignore as an action-preference variable value, which is already being tracked in #1759.

  • This issue's original topic, the misbehaving Test-Connection cmdlet, which mistakenly uses the PSHOST tag in its calls to .WriteInformation.

The solution to the latter is to simply omit the tag, as the following example demonstrates:

# With $InformationAction at its default, 'SilentlyContinue', this invocation is silent.
# If you set it to 'Continue', 'foo' prints.
& { [CmdletBinding()]param() $PSCmdlet.WriteInformation('foo', [string[]] @()) } 
@tibmeister

This comment has been minimized.

Copy link

@tibmeister tibmeister commented Sep 19, 2018

So I came across this because I am experiencing the same issue where -Quiet is not being respected. I looked at the code and it's a little difficult to follow and very over-complicated for what it’s doing, but I didn't see anywhere that is actually looking for this as a matter of suppressing output, nor am I seeing where it's switching from outputting a string to a boolean, which is what should be outputted when the -Quiet parameter is passed.

I did submit a PR last year (#2537) to add Test-Connection to the code and that was rejected at the time because “@PowerShell/powershell-committee would not accept this PR as it would introduce compatibility problems in the future”, so I was surprised to see the inclusion of this cmdlet now but not with the code of the original PR but rather brand new code that doesn’t provide all the expected functionality.

The biggest issue I come across is in my scripts I perform checks like this, “if(Test-Connection 8.8.8.8 -Quiet)” to branch into my logic, but with the -Quiet parameter not being respected, the branch always returns True because there’s not a False or Null. So this makes the command still completely unusable to me, and also since it’s been included in new releases of PowerShell, that makes upgrading very touchy for me. Please, fix this as it looks like it’s been several months since the issue was first reported. Either that, or re-instate the original PR, doesn’t matter as long as the functionality is returned.

I did submit a PR last year to add Test-Connection to the code and that was rejected at the time because they didn't want to add legacy cmdlets, so I was surprised to see the inclusion of this cmdlet now but not with the code of the original PR that was complete and functioned exactly as the "legacy" version did.

@alexandair

This comment has been minimized.

Copy link
Contributor

@alexandair alexandair commented Sep 19, 2018

If we have Test-Connection in 5.1 and 6.x, I would expect them to have the same output. I know that implementation is different, but users should not care about that. The current Test-Connection in 6.1 behaves differently giving us a different output then in 5.1. Besides that, -Quiet parameter is practically useless.

thom-s added a commit to thom-s/netsec-ps-scripts that referenced this issue Feb 12, 2019
Switching from Write-Outpute to Write-Verbose
Supressed Test-Connection output thanks to PowerShell/PowerShell#6768 (comment)
@NJ-Dude

This comment has been minimized.

Copy link
Author

@NJ-Dude NJ-Dude commented Apr 28, 2019

It seems this problem still stands, as of right now (PSVersion = 6.2.0) the cmdlet continues to display the "ping" information even if QUIET is present (adding "-InformationAction Ignore" does the trick but it means modules/scripts using the Test-Connection cmdlet will have to be updated, not cool)

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Apr 28, 2019

Would be really nice if we could get this fixed for 7.0. I'm more than happy to contribute the code, but we need an implementation spec to follow as there was very little agreement on earlier attempts to improve this.

cc @SteveL-MSFT @iSazonov

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Apr 29, 2019

I come back to this from time to time. Now I believe that we could solve this by using an explicit separation interactive and non-interactive scenarios with -Intercative switch. With the parameter we could implement rich user-friendly console output. It seems it is not too inconvenient for the user to type this parameter (as "-i"). Without the switch we'd do strong typed output without verbous console output that is good for script scenarios.

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Apr 29, 2019

Given that no other cmdlet uses such a parameter, I don't think it makes sense to have such duality. A cmdlet should do it's task and behave consistently; Interactive behaviour should not be separated from how it otherwise behaves.

For example, look at Get-ChildItem. Interactively, it in very useful thanks to the default formatter display. No changes are needed to also make it useful for automation purposes; the same command that works interactively also works in a script.

That's unnecessary complexity, I feel.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Apr 29, 2019

We could do the interactive output by default and enhance Quiet parameter to suppress the output in scripts.

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Apr 29, 2019

Again... I don't see the need to have a different output interactively vs in scripts.

If the cmdlet simply behaves like other cmdlets and outputs unusable data instead of being completely unique and outputting all its data in a difficult to capture or parse text stream (this is PowerShell, not Bash), there is absolutely no need for a change in behaviour.

-Quiet is a switch used by the original cmdlet in Windows PowerShell to give a pure True/False response, instead of outputting objects. Breaking that convention is a bad idea, in my opinion.

This cmdlet should behave in a manner consistent with existing cmdlets. There is no reason to have it alone use the current behaviour. In its current iteration it behaves more as one would expect a Unix utility to operate, very different to how other PowerShell cmdlets operate.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Apr 29, 2019

Again... I don't see the need to have a different output interactively vs in scripts.

Can you demo desired output in all supported scenarios? Note that meta information that the cmdlet outputs is very important.

outputting all its data in a difficult to capture or parse text stream

The cmdlet does strong typed object output. (The problem is that it is impossible to build these objects and display simultaneously because there is "meta" information.)

-Quiet is a switch used by the original cmdlet in Windows PowerShell to give a pure True/False response, instead of outputting objects. Breaking that convention is a bad idea, in my opinion.

That Windows PowerShell cmdlet only supports ping for which the concept of connection does not exist. This was initially controversial design. Right name for them would be Test-Ping ot Test-ICMP.
Current cmdlet supports ip "connections". Although I would prefer something like "Test-Connectivity".

In its current iteration it behaves more as one would expect a Unix utility to operate, very different to how other PowerShell cmdlets operate.

No, the cmdlet does strong typed object output. And the console output was made to look like utilities. But at the same time, you may see that in fact it is richer and more useful.
The problem is that this output cannot be obtained using the capabilities of the formatting subsystem and it is necessary to make a direct output to the console. (Note that this does not get mixed up with output objects in the output stream)

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Apr 29, 2019

The output can be obtained with the formatting system, if we structure the data objects in a more robust manner. I submitted a PR with a prototype already. A cmdlet that displays data in a form that doesn't properly reflect the underlying object data (e.g., by displaying data from subproperties instead of properly structuring the object class) is generally misleading and in my mind to be avoided wherever possible.

I will examine this further and put together a more complete example of what I perceive to be desired output. Test-Connection in Windows PowerShell may have been a controversial design, but I think it was a step in the right direction, if a very incomplete one.

@fMichaleczek

This comment has been minimized.

Copy link

@fMichaleczek fMichaleczek commented Apr 29, 2019

@iSazonov I wasn't agree with you after the first read BUT after testing, I understand your point of View

$a=Test-Connection www.microsoft.com 
$b=Test-Connection www.microsoft.com -Quiet

The $a and $b values are what I expect BUT I don't want the extra output.

Select-String has also a Quiet Parameter and it has nothing to do with "interactive" behavior

I'm agree that this command need an additional parameter to change the behaviour (Quiet or not).

I prefer the parameter 'Interactive' not by default but maybe a switch parameter "NoInteractive" is a better adjustment to let priority on interactive usage.

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Apr 29, 2019

Alright, here's what I would consider to be a somewhat more useful implementation.

General Points

  1. All current host/information output is relegated to the -Verbose stream. Currently that isn't used at all, and this is a perfect use case for it.
  2. No progress bar unless specified with a -ShowProgress switch.
  3. Remove the -Ping switch (it is the default behaviour).

Primary Output

Test-Connection www.google.com

  • Information from the Replies property of the output object should be included in the main output object, and primary output mode should be multiple objects, each representing a single ping attempt/reply object.
  • Buffer data is generally irrelevant, as it cannot be specified, and only a BufferSize property should be exposed. Replies.Buffer property should remain private.
  • Replies.Options property should be hidden from default formatting.
  • Results output as a table, grouped by Destination address (in the case that multiple destinations are specified).

Output visual mockup

Command used:

$Result = Test-Connection www.google.com
$Data = foreach ($Reply in $Result.Replies) {
    [PSCustomObject]@{
        Source = $Result.Source
        Destination = $Result.Destination
        Address = $Reply.Address
        RoundtripTime = $Reply.RoundtripTime
        BufferSize = $Reply.Buffer.Length
        Options = $Reply.Options
    }
}
$Data | Format-Table -GroupBy Destination -Property Source, Address, RoundtripTime, BufferSize

Resulting output:

   Destination: www.google.com
Source  Address       RoundtripTime BufferSize
------  -------       ------------- ----------
WS-JOEL 172.217.2.132            36         32
WS-JOEL 172.217.2.132            21         32
WS-JOEL 172.217.2.132            25         32
WS-JOEL 172.217.2.132            25         32

Test-Connection www.google.com -TraceRoute

  • Each hop should be output as a separate object, each containing the PingReply objects as a property accessible by hidden from formatting.
  • Main TraceRouteResult object should contain either ETS or class properties that calculate summary data from their four PingReplies.
  • Note: This object type we're using is currently bugged, and all PingReply objects report TtlExpired as their status. Recommend investigating progress of fix for .NET Core 3, or designing custom solution for TraceRoute backing to resolve the issue.
  • Output as a table, grouped by DestinationHost (Why is this property name different to that of the other object type used for standard pings?)

Output visual mockup

Command used:

$Result = Test-Connection www.google.com -TraceRoute
$Data = foreach ($Reply in $a.Replies) {
    [PSCustomObject]@{
        Hop = $Reply.Hop
        Source = $a.Source
        Destination = $a.DestinationHost
        DestinationAddress = $a.DestinationAddress
        Replies = $Reply.PingReplies
        RoundtripTimes = $Reply.PingReplies.RoundtripTime
        HopAddress = $Reply.PingReplies[0].Address
        BufferSize = $Reply.PingReplies.ForEach{$_.Buffer.Length}
        Options = $Reply.PingReplies[0].Options
    }
}

$Data | Format-Table -GroupBy Destination -Property Hop, RoundtripTimes, DestinationAddress, HopAddress, BufferSize

Resulting output:

   Destination: www.google.com
Hop RoundtripTimes DestinationAddress HopAddress     BufferSize
--- -------------- ------------------ ----------     ----------
  1 {0, 0, 0}      172.217.2.132      192.168.22.254
  2 {0, 0, 0}      172.217.2.132      75.144.219.238
  3 {0, 0, 0}      172.217.2.132      96.120.37.17
  4 {0, 0, 0}      172.217.2.132      96.110.136.65
  5 {0, 0, 0}      172.217.2.132      69.139.180.170
  6 {0, 0, 0}      172.217.2.132      68.85.127.121
  7 {0, 0, 0}      172.217.2.132      68.86.165.161
  8 {0, 0, 0}      172.217.2.132      68.86.90.205
  9 {0, 0, 0}      172.217.2.132      68.86.82.154
 10 {0, 0, 0}      172.217.2.132      66.208.233.242
 11 {0, 0, 0}      172.217.2.132      0.0.0.0
 12 {0, 0, 0}      172.217.2.132      216.239.59.124
 13 {0, 0, 0}      172.217.2.132      216.239.59.61
 14 {32, 28, 20}   172.217.2.132      172.217.2.132

I am firmly of the belief that if we're presenting data to the user, it should be readily accessible in a programmatic fashion, with similar surface structure to what is being displayed onscreen. Renaming of properties or burying data one or two levels deep in a property of the output object only invites confusion, bug reports, frustration, and a significant decrease in overall usability.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Apr 29, 2019

Oh, @vexx32 I see you never diagnosed a network. Your proposal was implemented by me in the first step and then rejected as not suitable for use in an interactive session. For example, we can look at a blank screen for a very long time after running a command Test-Connection www.google.com -TraceRoute. So implementation was changed to show an output (string or progress bar) for every response.

All current host/information output is relegated to the -Verbose stream. Currently that isn't used at all, and this is a perfect use case for it.

My suggetion above was to introduce Interactive switch to split interactive and script scenarious. You suggest do the same with Verbose switch which is even more unnatural practice.

No progress bar unless specified with a -ShowProgress switch.

String output and progress bar is in current implementation as two alternatives. We need only one. Progress bar is used in Windows PowerShell cmdlet. My preference is string output in interactive session. It is much more convenient.
And we never suppress a progress bar with a switch. We have $ProgressPreference for script scenarios. Some cmdlets shows a progress bar only for long operations by timer.

Remove the -Ping switch (it is the default behaviour).

Best practice is to use explicit parameters in scripts. It makes code more readable. It was not necessary in Windows PowerShell cmdlet where only ping was implemented. New cmdlet implements more functionalities and we need new explicit parameter for every ones.

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented May 9, 2019

@SteveL-MSFT I'd be happy to. :)

@NJ-Dude

This comment has been minimized.

Copy link
Author

@NJ-Dude NJ-Dude commented May 9, 2019

I love the sound of that.
Thanks

@tibmeister

This comment has been minimized.

Copy link

@tibmeister tibmeister commented May 9, 2019

Kinda interesting that PR 3125 covered all of this but the use of Test-Connection was rejected, but now we’ve come full circle. What about looking back at 3125?

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented May 9, 2019

Looking at it briefly, it looks like it essentially replaced Test-Connection with a differently-implemented command on Unix platforms to try to emulate the Windows command? Am I reading that right?

I don't think that's the best option available to us; consistency across both platforms in the implementation of the command as a whole is more valuable. It may have some interesting ideas we can utilise though, I'm sure!

I'll drop a link to the draft RFC when I'm done writing it & feel free to comment, adjust, etc... I'd love to hear some more viewpoints on this. 🙂

EDIT: PowerShell/PowerShell-RFC#172

@tibmeister

This comment has been minimized.

Copy link

@tibmeister tibmeister commented May 9, 2019

My specific use case for prompting this was based on the desire to use PowerShell Core on Linux, but the implementation was fully tested across both Windows and Linux. It was meant to replace the missing command in it's entirety.

@poundy

This comment has been minimized.

Copy link

@poundy poundy commented Jun 18, 2019

When will we see this? In 6.2.2? When will that likely land?
(was funny to see this thread raged from April 2018 to now over -quiet being noisy. Seems such a no-brainer breaking change)

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 18, 2019

I can get this code written pretty easily I think, I'm just waiting on the RFC to be accepted. Soon as that happens I'll get it done and submit a PR for this. 😄

@poundy

This comment has been minimized.

Copy link

@poundy poundy commented Jun 18, 2019

Oh I thought the status was that it was approved (not knowing exactly what states there are or what the full process is). But thanks for the update. Still a pity that it's taken over 12 months to make it quiet :)

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Jun 19, 2019

over 12 months to make it quiet

I expected more feedback

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Jun 20, 2019

@vexx32 you can start coding this and put it behind an Experimental flag in case any feedback comes in that changes the current proposal

@SteveL-MSFT SteveL-MSFT added this to the 7.0-Consider milestone Jun 20, 2019
@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 20, 2019

@SteveL-MSFT I've already got a mostly-working implementation. I'll look to submit a PR w/ some Experimental flags soon so we can talk about the code more concretely. 💖

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Jun 20, 2019

After thinking in last days about the desired behavior, I would prefer to have interactive behavior with a minimum of parameters by default (fast typing and user friendly display), which would be convenient in an interactive session. And converting to script style with additional parameters (this implies different types in output).

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 20, 2019

Could you elaborate a bit more on how you think that could work @iSazonov?

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Jun 21, 2019

@vexx32 Do you ask about implementation or UX design?

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 21, 2019

Mainly what you think the UX would be like for that, I think. :)

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Jun 21, 2019

In interactive session best UX is

  1. minimal typing
    It means:
  • ping is default
  • switching to another mode (traceroute and etc.) with one parameter
  1. user friendly output
    It means:
  • emulate ping.exe (tracert.exe and others) output on console host like I tried in the demo code - with headers, footers and informative lines well formatted. No need to think about output types - they is not used, only displayed.
  • add parameters to switch to script mode - that is to suppress output of user-friendly text on console host and emit strong typed objects. No need to format this output. We discussed Quiet which returns True/False but we need a parameter(s) to emit other raw strong typed objects (like -RawOutput). It is acceptable UX to use additional parameters in scripts.
@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 21, 2019

Thanks, I think I understand what you're getting at a bit better now.

I don't really see the need for a dual mode like this, though? No other cmdlets in PowerShell have this split between interactive and "script mode" parameters.

If you wanted the exact output of ping / tracert, why wouldn't you just use those utilities directly?

PowerShell has never made a significant effort to completely mimic an existing command; I think Get-ChildItem is probably the closest, but it's almost the only one to do so.

If we wanted to thoroughly emulate ping / tracert display like you say, I would suggest we instead have that as a separate cmdlet or function, e.g., Show-Connection, rather than clutter Show-Command with extra parameters that have no existing precedent or need within PowerShell.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

@iSazonov iSazonov commented Jun 21, 2019

I don't really see the need for a dual mode like this, though? No other cmdlets in PowerShell have this split between interactive and "script mode" parameters.

We have gaps in out formatting system. For example, we have an issue with request to have header/footer dir. There are other scenarios where it would be convenient.

If you wanted the exact output of ping / tracert, why wouldn't you just use those utilities directly?

I do :-). These native utilities are very powerful because they are low-level but they are frozen. We can do smarter things than they do - this is the essence of the appearance of this cmdlet (and not just to make a ping) - if you take a deep look at how the addresses and names in the cmdlet are now formed and how they are output, you will see that there are much more useful than in the native ping. It looks like tricks but it is really very useful.

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Jun 21, 2019

Of course we have gaps in the formatting system. But I don't think the answer is to implement a custom solution every single time. That just creates more problems and makes it even more difficult to improve or rewrite the formatting system to be more effective across the board.

Yeah, you've done some great work with it! I very much appreciate that. The current mode of output is not useful for anything but interactive use. We could simply remove the output step and call it Show-Connection and then utilise what you've written in a more PowerShell-focused solution for Test-Connection itself as I outline in the RFC.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

@SteveL-MSFT SteveL-MSFT commented Oct 3, 2019

@vexx32 I just tried your PR and it improves it quite a bit, but I noticed that in Windows PowerShell, it emits PingReply individually to the pipeline so you get something that looks like:

PS C:\Users\slee> Test-Connection localhost

Source        Destination     IPV4Address      IPV6Address                              Bytes    Time(ms)
------        -----------     -----------      -----------                              -----    --------
SLEE-DESKTOP  localhost       127.0.0.1        ::1                                      32       0
SLEE-DESKTOP  localhost       127.0.0.1        ::1                                      32       0
SLEE-DESKTOP  localhost       127.0.0.1        ::1                                      32       0
SLEE-DESKTOP  localhost       127.0.0.1        ::1                                      32       0

However, with your change, all the ping replies are members of a single object:

PS> Test-Connection localhost

Source       Destination Replies
------       ----------- -------
slee-desktop localhost   {System.Net.NetworkInformation.PingReply, System.Net.NetworkInformation.PingReply, System.Net.NetworkInformation.PingReply, System.Net.N…

Are we not able to get back the Windows PowerShell behavior?

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Oct 3, 2019

@SteveL-MSFT That object output has not changed from the initial implementation in PS Core; the success output has always had the single object. 🙂

As mentioned in the closing comments of the PR, that is only a partway implementation of the RFC to help simplify reviews. I will be submitting a follow-up PR shortly. Just have to rebase that branch to remove the now-duplicate commits and submit the rest of it to get much closer to true parity with Windows PowerShell's implementation. It will still differ somewhat (as can be seen from the RFC we reviewed a handful of weeks back) but will be hopefully much more versatile. 😊

@vexx32

This comment has been minimized.

Copy link
Collaborator

@vexx32 vexx32 commented Oct 3, 2019

@SteveL-MSFT see #10697 for the next chapter in this adventure! 😊

@msftbot

This comment has been minimized.

Copy link

@msftbot msftbot bot commented Oct 23, 2019

🎉This issue was addressed in #10478, which has now been successfully released as v7.0.0-preview.5.🎉

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.