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

sorting parameters by position then by required and then by name in syntax generation #19

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

rymir75
Copy link
Contributor

@rymir75 rymir75 commented Apr 27, 2016

Two cases when it's needed:

  • when you use R# and sort class members by type, visibility and name
  • when you introduce a base cmdlet class with few base parameters (positioned)

Additionaly MSDN doesn't guarantee that Type.GetMembers will return array of members in order of declaration
https://msdn.microsoft.com/en-us/library/k2w5ey1e(v=vs.110).aspx

@ChrisLambrou
Copy link
Contributor

Thank you for taking the time to check out the code and submit a PR. However, I don't really understand why the order that the parameters are defined in the .dll-Help.xml MAML help file has any bearing on either of the cases you've mentioned.

Two cases when it's needed:

when you use R# and sort class members by type, visibility and name

Are you referring to the the C# source editing feature of ReSharper, whereby it will order the fields and properties in its auto-completion lists, for example? If so, I don't see how controlling the ordering of the documentation in the MAML help file has any influence over this.

Are you referring to a PowerShell editing feature of ReSharper? If so, it's something I'm unaware of.

when you introduce a base cmdlet class with few base parameters (positioned)

Yes, this PR would sort the parameters irrespective of them being defined in the cmdlet class or a base class, but why does this matter? This would simply control the declaration order of the parameter elements in the MAML help file, but I'm not aware of any consumer of the file (e.g. the Get-Help cmdlet) that is sensitive to that ordering.

Could you please clarify in more detail exactly why the ordering of the parameters in the MAML help file is important, with a specific example?

@rymir75
Copy link
Contributor Author

rymir75 commented Apr 27, 2016

I propose you an experiment. I'm sure that you have some MAML help file. Go to the syntaxItem element and change the order of positioned parameters e.g. move parameter with position 0 after a parameter with position 1, save it. Now, check how Get-Help renders syntax for changed cmdlet. Order of parameters in syntaxItem element has direct influence on generated syntax by Get-Help. It can mislead a user of the cmdlet.

This feature of R# is called "File Layout". If you have it configured that way that during code clean up you sort members of the class it can interchange properties e.g. property ParA (position 1) will be above property ParB (position 0). Similar effect will be if you have class hierarchy: "B" inherits from "A" and "A" inherits from "Cmdlet" and "A" has parameter ParA (position 0, type string) and "B" has parameter ParB (position 1, type string) then syntax for cmdlet will be: [-ParB] [-ParA] but should be exactly opposite.

I suppose that System.Type.GetMembers(BindingFlags) looks for members in the order they are declared and when we have deeper class hierarchy it first looks for members declared in the type and after it his parent and so on. Nevertheless MSDN documentation doesn't guarantee any order neither alphabetical nor declaration so we shouldn't depend on it.

I hope this clarifies my PR.

@ChrisLambrou
Copy link
Contributor

Ah, I think I get it now. Get-Help detects the presence of the MAML help file, it displays its contents regardless of whether or not they actually match the API of the corresponding cmdlets. Honestly, the Get-Help cmdlet really isn't very good. I'm sorely tempted to dig out its source code and fix all the many, many problems I've found with it. Furthermore, the order parameters are declared in the source code may not match the explicit ordering defined by the Parameter attributes, so we should explicitly order them according to the Parameter attribute properties and the parameters' names, rather than the arbitrary order returned by Type.GetMembers.

Okay, will have another look at the code as there's one minor issue I wanted to dig into, and then hopefully merge it soon. Thanks again for this PR. 😄

@@ -324,7 +324,9 @@ private XElement GenerateSyntaxItemElement(ICommentReader commentReader, Command
{
var syntaxItemElement = new XElement(CommandNs + "syntaxItem",
new XElement(MamlNs + "name", command.Name));
foreach (var parameter in command.GetParameters(parameterSetName))
foreach (var parameter in command.GetParameters(parameterSetName).OrderBy(p => p.GetPosition(parameterSetName)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p.GetPosition returns a string, which may be any of the following:

  • "n", where n is an integer, taken from the Position property of the Parameter attribute, if it is specified.
  • "named" if the Position property is equal to int.MinValue, which is the default value if the Position hasn't actually been set.. I cannot remember under what circumstances this might happen.
  • null if XmlDoc2CmdletDoc is unable to retrieve a value for the current parameter set being documented.

This rather relies on parameter positions being single digits, and that "1" and "2", for example, are lexicographically lower than "named" and null. This is probably good enough in most circumstances, but I'll probably revisit this area of the code at some point to make things a bit more explicit.

@ChrisLambrou ChrisLambrou merged commit eef6770 into red-gate:master Apr 27, 2016
@ChrisLambrou
Copy link
Contributor

Cheers! This has now been published to NuGet, and I've updated the contributors list in the project readme. 😸

@rymir75
Copy link
Contributor Author

rymir75 commented Apr 27, 2016

Now, I can show off to my wife 😃
It was a pleasure and my first contribution to open source.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants