search

SeeminglyScience / EditorServicesCommandSuite

Collection of editor commands for use in PowerShell Editor Services.
Other
151 stars 13 forks source link

Feature #29 splat expression #30

Closed Jawz84 closed 6 years ago

Jawz84 commented 6 years ago

Request For Comments. This is my line of thinking so far, and initial testing is looking OK. Let me know if i should go on in this direction, or if you have other ideas. Thanks!

SeeminglyScience commented 6 years ago

Thanks again for taking this on @Jawz84! Excited to get this in :)

Logic looks sound, the trick will be tying it into the existing script writer logic. My first thought was you could add your own ParameterBindingResult to boundParameters.BoundParameters but it doesn't look like there's a public way to create a ParameterBindingResult object.

I'm thinking the route to go is to create our own class that sort of mirrors StaticBindingResult and ParameterBindingResult so additional parameters can be added.

The custom class can then be created from the result of StaticParameterBinder.BindCommand and used for the rest of the refactor.

Jawz84 commented 6 years ago

At this point, this is a working prototype. I have not tried adding to the BoundParameters collection, I've just used the flexibility of the PowerShellScriptWriter splatWriter. I'd love to get some tests going, to check edge cases consistently.

I went ahead and omitted the Common Parameters from the splat. Maybe someone finds it useful to have an additional parameter: -IncludeCommonParameters, but I never use all of the common parameters at the same time, so i'm leaving that for someone else for now.

Something that I found when working on that: I needed a list of all Common Parameters. They are inside System.Management.Automation.Internal.CommonParameters, but I can't find out how to use that properly yet. That's why I implemented my own list, which feels kind of meh. Also because the optional risk mitigation common parameters are missing (WhatIf and Confirm). If you have insights on that, I'm all ears.

SeeminglyScience commented 6 years ago

Awesome! Haven't gotten to give it a test drive yet, but I'll give it a spin this weekend :)

For common parameter names, they are stored in a HashSet<string> via the static property System.Management.Automation.Cmdlet.CommonParameters. There's also Cmdlet.OptionalCommonParameters for WhatIf, Confirm and Transaction. I 100% agree with the decision to skip common parameters, good call :)

Jawz84 commented 6 years ago

System.Management.Automation.Cmdlet.CommonParameters awesomesauce! That's what we needed!

I wrote logic to do rudimentary parameterset matching and I've refactored that logic into a function. It looks a lot better that way. The output is in such a way, that implementing the 'Mandatory parameters' only feature is probably a breeze. I made a prototype in PowerShell first, but in converting that to C#, I'm running into problems with my Linq expressions. I'll ask around, and I'll commit my progress thus far here.

Jawz84 commented 6 years ago

Lots of things fixed. With thanks to @indented-automation :-) There is at least one bug that I know of with the -MandatoryParameters switch though. I'll add more detail later.

Jawz84 commented 6 years ago

Barring tests, the implementation of -AllParameters and -MandatoryParameters is ready for a good review. I've done testing manually for now. Tests to include:

cases :
[single-parameterset-command with no bound params] [multi-parameterset-command with no bound params] [multi-parameterset-command with a bound parameter from the __allparametersets parameter set] [multi-parameterset-command with a bound parameter that matches two parameter sets] [single-parameterset-command with a bound param]

cases :
[multi-parameterset-command with a bound param from one set]

cases:
[multi-parameterset-command with bound params from two parameter sets]

Jawz84 commented 6 years ago

All lines tested with both ConvertTo-SplatExpression -AllParameters and ConvertTo-SplatExpression -MandatoryParameters, results after // on each line.

# should result in splat based on default parameter set  //some failed 
Send-MailMessage   # //fail: @variablename is written in wrong place. 
Get-FileHash  # default Path parameter set only has -Path as mandatory parameter  //fail: @variablename is written in wrong place.
Get-ChildItem -Name  # -Name is in either parameter set. Default set does not have any mandatory params.  //pass
mkdir -Path "c:\test\test"     # path param is in nameSet and in pathSet parametersets, pathSet is default set. in pathSet, -Name is optional, -Path is mandatory.  //pass
Send-MailMessage -From "someone@someplace.com"  #//pass

# should result in splat based on selected parameter set //pass
mkdir -Name "somename"  # pathSet is the default parameter set, nameSet is the other set. If nameSet is active, -Path is optional. In pathSet, -Path is mandatory.

# should do nothing //fail: it splats the default parameterset
Get-FileHash -LiteralPath "c:\test\test" -Path "c:\test\test"
Jawz84 commented 6 years ago

All (manual) tests passed.

Jawz84 commented 6 years ago

@SeeminglyScience happy 'n proud to say, this is now ready for a review round! Manual testing OK. Implementing the tests xunit style will take some time.

Jawz84 commented 6 years ago

All tests passing. The work is 'done' and ready for review. Whenever you are ready :-)

Jawz84 commented 6 years ago

All done, tests passing. ;-) Take your time.

Jawz84 commented 6 years ago

Very happy with the merge and collab too! Really cool! I looked at your todo-list issue for 0.5.0, maybe I could help with some of that? It would have to be lower paced on my end than this contribution.

Op 19 aug. 2018 7:50 p.m. schreef Patrick Meinecke notifications@github.com:

@SeeminglyScience approved this pull request.

Looks amazing. Thank you so much for doing this and all your patience!

I still have a lot to do before I can release 0.5.0 and can't give an accurate guess as to when we'll get this out but I'll definitely keep you updated.

Seriously though, it's been a pleasure working you! Hope to again in the future :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/SeeminglyScience/EditorServicesCommandSuite/pull/30#pullrequestreview-147466050, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AZPSdQHt2hdN8C6sRg_3NsRFjp8n5QeFks5uSaVsgaJpZM4Vg3j_.

SeeminglyScience commented 6 years ago

@Jawz84 Absolutely! :D

Most of what is needed is updating the markdown help and documentation in general. There's a few new systems like more robust settings, the whole context based refactor system with Invoke-DocumentRefactor.

For the actual remaining commands, all the ones that are left require the DocumentEdit processor to be able to accept edits for multiple files. I'm working on that as part of ConvertTo-FunctionDefinition which is almost done. I'll give you a holler when that's in in case you'd like to work on one of those.

I also have a lot of ideas for new refactors now that they are a bit easier to develop with the new systems. I'll get some issues documenting them up soon :)