Cleanup TestConnectionCommand #10439
Conversation
| /// <summary> | ||
| /// The implementation of the "Test-Connection" cmdlet. | ||
| /// </summary> | ||
| [Cmdlet(VerbsDiagnostic.Test, "Connection", DefaultParameterSetName = ParameterSetPingCount, HelpUri = "https://go.microsoft.com/fwlink/?LinkID=135266")] |
There was a problem hiding this comment.
Using ParameterSet prefixes looks good to me - this makes code more readable and understandable.
Otherwise a code reader will have to review all code in the file to find that a const is used in Parameter() attribute as a parameter set.
I'd remove the commit.
There was a problem hiding this comment.
Is the <Name>Set form not sufficient to convey that intention here? Every context they're used is either in a [Parameter] attribute or in a switch (ParameterSetName) construct -- both of which are more than sufficient to indicate that they're used in parameter sets.
Since attribute values must be constant values, I think it's reasonable to assume that readers will be able to infer this information. (Plus, most code editors have a "Show/Peek Definition" in their right-click context menu, and these constants are defined immediately below here, at the top of the class.)
As I was reading through this file I found that the overly long names of the constants tended to make them confusing to read, and they're typically surrounded by at least one mention of [Parameter] or something similar, rendering the Parameter segment of the name pretty redundant in context.
It's also not a pattern I see anywhere else in the repo?
There was a problem hiding this comment.
C# best practice is to use informative names without abbreviations. As result we sometimes have long names.
I see different patterns in our code. You could look AddMember.cs, AddType.cs, ConvertFromMarkDownCommand.cs.
It would be nice to have one patter but we haven't.
As for my request one example is that we frequently use switch statements for parameter sets and it would be good at first glance to understand that a constant with the parameter set name is used there.
There was a problem hiding this comment.
I guess that makes some sense, and yeah I hear you on the inconsistent patterns there. With the switch statements, these are usually used as switch (ParameterSetName) {}, so I think it's pretty clear there as well given that that's the context of the switch statement.
There was a problem hiding this comment.
Adjusted the names here to be clearer. 🙂 👍
| /// <summary> | ||
| /// Force using IPv4 protocol. | ||
| /// </summary> | ||
| [Parameter(ParameterSetName = DefaultPingSet)] |
There was a problem hiding this comment.
I'd do not remove this - later we could add new parameter sets in the cmdlet. I did not implement them because of bugs in CoreFX.
There was a problem hiding this comment.
Which possibilities exist that would be agnostic to IPv4/v6?
There was a problem hiding this comment.
I don't understand you question. I mean that later it would be easier to add new features in the cmdlet (like PathPing). (I am thinking about enhanced version of the cmslet with remoting support: user in A point could ping/traceroute/... C from B)
There was a problem hiding this comment.
Is there significant advantage to having that from this cmdlet? This would have to be supported by PSRemoting anyway, so I don't know why we'd do that here -- users have Invoke-Command already, and the native APIs utilised by .NET core (and by extension, us) don't have this capability, so we'd be leaning on that already.
Basically, I'm asking -- why do these parameters need to have the sets specified when not all parameters are written this way? I think it makes more sense to have the command written to its current capability, and then when we implement new features we can add them or adjust parameters as needed.
Can we reasonably predict possible future scenarios here with accuracy? I don't think it makes a lot of sense to complicate parameter binding here at this point in time. 😕
There was a problem hiding this comment.
🤷♂ not particularly an issue I suppose, we could debate this all day. It isn't hurting anything by leaving it in; I reverted this commit.
Thanks! 🙂
There was a problem hiding this comment.
Thanks!
This would have to be supported by PSRemoting anyway
It seems we have a tracking issue to have remoting common parameters for all cmdlets.
With this we could get functionality like Test-NetConnection.
There was a problem hiding this comment.
That would be awesome!
Something to note: The original implementation has a -Source command. It seems the win32 APIs for ICMP echo actually support a similar parameter, but there it is used to designate the NIC that is used to send the ICMP request.
However, .NET Core does not expose this capability to us at present. Hopefully we can get that in future as well! 😊
There was a problem hiding this comment.
It seems the win32 APIs for ICMP echo actually support a similar parameter,
It is WMI.
| /// The implementation of the "Test-Connection" cmdlet. | ||
| /// </summary> | ||
| [Cmdlet(VerbsDiagnostic.Test, "Connection", DefaultParameterSetName = DefaultPingSet, HelpUri = "https://go.microsoft.com/fwlink/?LinkID=135266")] | ||
| [OutputType(typeof(PingReport), ParameterSetName = new string[] { DefaultPingSet })] |
There was a problem hiding this comment.
It do not make the code more readable for me.
I'd remove the commit.
| /// </summary> | ||
| [Cmdlet(VerbsDiagnostic.Test, "Connection", DefaultParameterSetName = DefaultPingSet, HelpUri = "https://go.microsoft.com/fwlink/?LinkID=135266")] | ||
| [Cmdlet(VerbsDiagnostic.Test, "Connection", DefaultParameterSetName = DefaultPingSet, | ||
| HelpUri = "https://go.microsoft.com/fwlink/?LinkID=135266")] |
There was a problem hiding this comment.
Why do we need the new line? I'd revert.
There was a problem hiding this comment.
Mostly readability, long lines tend to get lost in the mix here. It's also a common pattern with a fair few cmdlets in the repo to put the HelpUri on the following line.
🤷♂️
There was a problem hiding this comment.
I believe text width up to 120-144 is good for modern displays.
There was a problem hiding this comment.
Sounds good, I'll revert this one. 🙂
There was a problem hiding this comment.
@iSazonov the width here if this stays on a single line is 150; should I leave this on two lines? 😕
There was a problem hiding this comment.
My request is not blocking. If you see a problem on your screen - make two line or multi-line format.
There was a problem hiding this comment.
I think I'll leave this one as-is for now. Two lines is (I think) sufficient. Thanks! 🙂
|
We're ignoring Codacy's complaints per @iSazonov's comment here: #10439 (comment) It flags the type specifier as unnecessary, which is technically correct, but stripping it out isn't particularly elegant in this context. |
|
@adityapatwardhan is there anything more you need from me on this one? Would like to get this one wrapped up soon if possible, I have a few follow ups to this I would like to get done by 7.0 release. Thanks! 💖 😊 |
|
@vexx32 Thank you for your contribution! |
|
🎉 Handy links: |
PR Summary
Style pass only as part of #10044
PR Context
Rewriting the Test-Connection Command a good bit. @iSazonov suggested I split it up into stages to make reviewing the PR changes much easier.
This is part 1, only containing style fixes. There are no functional changes.
Feel free to review commit-by-commit for easier diffs. 🙂
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.