Removing Dynamic parameters

[jhoneill]

PR Summary

Following a suggestion from Sebastian Schütze / @RazorSPoint on twitter I'm sending PR for the work I've done replacing Dynamic parameters with classes. This requires a change in the loading order (otherwise the classes are not visible when completing the command) and which in turn would break the existing pester tests so I have made some changes to mock something close to the rest-api, which will be in scope for mocking, where the functions called by the classes won't.

The reason for removing large scale dependence on dynamic parameters is the performance hit for tab expansion. With the module loaded type get- [tab] with the 6.44 release loaded and compare my fork to see the difference.
·

PR Checklist

• [ O ] [Write Help]
  No functionality Added, so no help needed :-)
• [ O ] [Write Unit Test]
  Fixed existing unit tests. Note that ~ 20 tests don't work with the 6.44 release and I have not fixed these
• [ X ] [Update CHANGELOG.md]
  Done

[SebastianSchuetze]

@jhoneill I think your master branch is out of date compared to ours. Could you try to sync the newest from this repo?
I don't know if it causes too much trouble for you, but you are 154 commits behind this repo. It causes also too many conflicts. Hope you don't mind otherwise it is hard to see what really changed and whatnot. But anyways thanks already!

I also have to say that I am currently not fully understanding the dynamic parameters problem. Do you have some good references that can help me?

[SebastianSchuetze]

@DarqueWarrior I hope you remember the tweet thread. This was about the performance problems with dynamic parameters. This is a bigger substantial change so I would need your opinion on that.

@jhoneill please have patience here, because it is Donavan's module and he has the last say in these kinds of things.

[jhoneill]

@jhoneill I think your master branch is out of date compared to ours. Could you try to sync the newest from this repo?

Fixed that. Actually the code was up to date but the commit history was broken. .

The dynamic parameters problem.
Put simply: type get- and press tab. Then load the module. and repeat. Nothing seems to happen, but wait, wait some more and keep waiting and then ta-da it works. So what's going on ... ?

When you have a parameter which you want to both validate and tab complete, normally you either define an enum or a validate set but if the values aren't fixed.... like names of your printers, or projects in AzDO or anything else the author can't know ?
You can use an argument completer, (see Register-ArgumentCompleter which has been built into PS since, IIRC, V4, but was in a module called tabExpansionPlusPlus before that). But completers only tab complete, so [tab] will fill in my project name but it won't catch "Porject1" instead of "Project1".
To solve this some people use dynamic parameters, and build a validate set at run time. They're awkward to use, and worse when there are lots. Firstly, if you move the dynamic parameter code out of the function to avoid duplicating it, you can see the parameters when you look at the function. Secondly, you get the perf issue: when PowerShell gets a list of commands gets their parameters - expanding the dynamic ones. 77 commands have project name - if it takes 1/10th second for each...

Using expansion & validation classes - described here
https://jamesone111.wordpress.com/2019/09/23/the-classy-way-to-complete-and-validate-powershell-parameters/ - side steps this problem, although there are some extra steps needed when loading PowerShell classes inside a module and accessing them outside; and a side effect is the pester tests which use InModuleScope does not apply mocks to function calls from inside the classes. So I had to move where the mocking nearer the web request.

[SebastianSchuetze]

Thanks! I haven't tried to read through your comment. And before you put effort into the failing unit tests. I want to first get @DarqueWarrior opinion on that.

In the meantime:

1. What is the minimal PowerShell version this is working with? TFS 2017 is supported and it should support at least the minimal requirements for the OS with TFS 2017
2. Did you only edit the dynamic parameters in these files and didn't change any other logic. (We have to review it anyway)

[jhoneill]

Thanks! I haven't tried to read through your comment. And before you put effort into the failing unit tests. I want to first get @DarqueWarrior opinion on that.

Of course. I didn't want to do the PR with new failures in the tests, so I used that as my check for was my stuff ready to share. [There are some failures with the download of 6.4.4, but I haven't added to them. I've also found the tests aren't totally self contained - some don't work properly unless an earlier test in the suite has run]. I made some changes to the tests to get them to run, and and found a small number of bugs/regressions which I'd introduced and fixed those. And yes I totally understand it is @DarqueWarrior 's module :-) sometimes other people's effort towards a module you maintain don't take it in the direction you want it to go or fixes an issue which dominates their thinking without seeing all the things you do (either or both of those could apply to what I've done here).

In the meantime:

1. What is the minimal PowerShell version this is working with? TFS 2017 is supported and it should support at least the minimal requirements for the OS with TFS 2017

The classes stuff requires at least PowerShell 5.0 I've been jumping back and forth between 5.1 and the preview of 7. I do the occasional test pass on 6 but I'm not actively using it.

2. Did you only edit the dynamic parameters in these files and didn't change any other logic. (We have to review it anyway)

I have made some other changes. The only functional one is I added some of the parameters of _callApi to the Invoke- command.

One of the problems when trying to understand people's code is when it calls a function you can't see. If it is big lump of code called from many places it needs to be pulled out, but sometimes the called function is only one line, and isn't the current file, and is perhaps only called from one place. There was one case where the code called went something like

if (_foo)  {... }

function _foo  {
if  (-not $bar)  { return $false}
else {return $true} 
}

the function could just be
{$bar -as [boolean]}
but calling a function just to return a variable... it makes for more readable code to just write
if ($bar)
if one makes a lot of use of mocks in pester there is a case for using the function because that can be mocked. That's a one of those cases where the originator of the code might take one view of the best way to do it, and someone new looking at their code might take a different one :-)

[MichelZ]

IArgumentCompleter! You learn something new every day. Brilliant, and much preferred to the DynamicParams.
Well done!

[jhoneill]

I've update my fork following on from a discussion PowerShell/PowerShell#12132

• Some ways of loading don't make the short cut of labelling the parameter [validateThing()] instead of [validateThingAttribute()] don't make the class visible so I've changed that.
• Also if the classes are in the PSM1 file and not in their own .ps1 loading the module with using module makes the classes visible to the wider PowerShell session so I've changed the build to put classes into the PSM1 which means they now longer need to be pre-loaded in the PSD1.
• I've removed the need for some of the classes to have some helper functions exported and then hidden.

[SebastianSchuetze]

Thanks for updating. Unfortunately, we make some changes to the unit tests and a lot of conflicts now exist.

[jhoneill]

Thanks for updating. Unfortunately, we make some changes to the unit tests and a lot of conflicts now exist.

I'll see if I can merge those changes down into mine ...

[jhoneill]

@SebastianSchuetze I made some additional changes in the unit tests, in particularly to get rid of the loading of files from ....\Source and ensuring the mocks worked with the loaded module.

[SebastianSchuetze]

Thanks for your effort. Currently @DarqueWarrior is investing in a PR #285 to split up nearly all unit tests. It could be that it takes a while and creates a lot of conflicts. Please also do not change so many different things in one PR. It makes it harder for us to check later.
We can't merge anything that we do not under stand. And the more changes are there the harder it gets. 🙂

Hope you understand.

[jhoneill]

Thanks for your effort.

You're welcome. Most of this is stuff I was doing anyway and after our exchange on twitter we said the best thing was to throw everything I had into the pot :-)

Currently @DarqueWarrior is investing in a PR #285 to split up nearly all unit tests. It could be that it takes a while and creates a lot of conflicts.

Understood. As part of that work, some consistency needs to be applied to the tests. Some tests rely on loading individual PS1 files from the sources directory based on its path relative to tests, so those tests (a) won't run against a built module - so you can't run the tests against previous versions and (b) need to know which class and function files to load and the directory layout i.e. they are tied to an implementation detail which they should not be. (c) Interfere with tests which run later.
So what I have changed ensures on a system with the module installed, one can load it with import-module or using module , and then run either xxx.tests.ps1 or invoke-pester and any single test should run alone or as part of suite. That change meant wrapping some existing tests in InModuleScope VSTeam {} and moving some mocks to the write place, and also included adding the environment variable "Testing" so validation classes don't consider test data to be invalid.

Please also do not change so many different things in one PR. It makes it harder for us to check later.

I totally understand. What I have done is

1. Added classes for argument completers and validators
2. Changed source\classes\classes.json to add these classes and to build the classes into vsteam.psm1 [this allows the classes to be visible outside the module when it is loaded with using module. Import-Module does not make them visible]. Modified build-module.ps1 to append source\vsteam.psm1 to the version which holds the classes.
3. Remove most of the Dynamic parameter blocks and inserted a normal parameter which uses the completers and validators. This is the same quite small thing done in many places. I've changed almost every file in source\public. It's a global change and unless it's done in groups of files it either means many small PRs or one big one.
4. I made some other changes - for example it was much easier for me to follow the code with some of the simple things like _getInstance replaced with [VSTeamVersions]::Account - but then I found that they were to allow something to be changed in testing with a mock, so I've backed most of those changes out.
5. In order to prove, to a reasonable degree that I had made my changes correctly I needed to run the unit tests... see above.

We can't merge anything that we do not under stand. And the more changes are there the harder it gets. 🙂

Hope you understand.

Of course. Dynamic parameters only become a problem when you have many of them, so you start down a perfectly reasonable track of using a dynamic parameter, and then another and eventually you have ~100 of them and typing get-[tab] takes 10-20 seconds to complete (or jumps into the debugger) because powershell is expanding every command's dynamic parameters.
It's classic technical debt - the cost of adding one more dynamic parameter is quite small and the cost of undoing the original decision gets bigger. So now you have this mad guy changing almost every function and you must be thinking "How am I meant to review all of this".

[DarqueWarrior]

First let me apologize for taking so long to engage on this PR. Reading the comments this sounds very good. I did an enormous rewrite of all the tests to remove the InModuleScope because it was causing more issues than it was solving. Also all the unit tests should have zero dependencies on each other. I will pull this PR down this week and start to review.

[DarqueWarrior]

One other note on tests. I noticed some mocking at the _callAPI level instead of the Invoke-RestMethod level. That might ease a lot of issues as well. Just have to make sure that _callAPI is tested 100% and rock solid.

[DarqueWarrior]

Something is wrong with this fork. When I run

.\Build-Module.ps1 -ipmo

using PowerShell 5.1 I get a bunch of errors and the module does not load.

Seems to work in 6 and 7. I don't have time to troubleshoot this at the moment.

[DarqueWarrior]

Just looking at the code I have to agree this looks MUCH better without the dynamic parameters. The ArgumentCompleter attribute is very clean. If we can get this merged with the lastest in Master now and fix the 5.1 load issue we should merge this one next. Once those issues are fixed I can do a final review.

Great work!

[SebastianSchuetze]

I have to agree! All that sounds really good, but I have to say that I didn't look at all the code like Donovan did.

[jhoneill]

@DarqueWarrior - no worries, I've seen you're massively busy, and this was a huge lump of a work to drop in your path.
Glad you like what you're seeing and I'll see if I can debug the the 5.1 issue. I've been building on 7 and testing the results on 5 and 6 so there may be something I need to address - I've found there are strange things about how powershell loads classes from a module, and it may be one of those (which I must write up at some point).

[DarqueWarrior]

@DarqueWarrior - no worries, I've seen you're massively busy, and this was a huge lump of a work to drop in your path.
Glad you like what you're seeing and I'll see if I can debug the the 5.1 issue. I've been building on 7 and testing the results on 5 and 6 so there may be something I need to address - I've found there are strange things about how powershell loads classes from a module, and it may be one of those (which I must write up at some point).

I completely understand. Classes have really been a pain in the ass in PowerShell. At some point in the future I might consider moving all the classes to a .net Core assembly and take a dependency on that assembly. That will allow us to use C# and have proper classes.

[DarqueWarrior]

If you merge the latest you will not have to do the Testing check.

[jhoneill]

@DarqueWarrior - no worries, I've seen you're massively busy, and this was a huge lump of a work to drop in your path.
Glad you like what you're seeing and I'll see if I can debug the the 5.1 issue. I've been building on 7 and testing the results on 5 and 6 so there may be something I need to address - I've found there are strange things about how powershell loads classes from a module, and it may be one of those (which I must write up at some point).

I completely understand. Classes have really been a pain in the ass in PowerShell. At some point in the future I might consider moving all the classes to a .net Core assembly and take a dependency on that assembly. That will allow us to use C# and have proper classes.

It turns out not be classes directly - PowerShell classes which are directly in the PSM1 file, not dot-sourced by the PSM1, are available outside the module, if the module is loaded with using module, instead of import - something else C# sidesteps. For that reason I merged the classes into the PSM1 and added the original PSM1 to the end. On PS5 the classes go out as ascii and then the PSM1 follows as unicode and lots of red ink follows. I changed the buildmodule.ps1 and mergefile.ps1 to ensure everything goes out as ASCII and it all seems good after a first check

I'll merge those changes in shortly!

[DarqueWarrior]

Is there anything I can do to help merge the last changes from master?

[jhoneill]

Is there anything I can do to help merge the last changes from master?

Hi. Just merged over 100 files from the source folder. I've checked the files in but I have not gone over the results properly yet.

Unfortunately many of the recent test changes seem to depend on functionality being in specific source files, and those files in a specific location relative to the unit tests. [that's what causes the "Cannot find the type for custom attribute 'ValidateProjectAttribute'." errors. The unit tests don't know they need to load a class file with a class in it. It wouldn't be a problem if they worked by loading the module] My hunch is most of the tests will be broken at the moment.

[DarqueWarrior]

I can fix the tests. Now that your code is in place I can take it from here. Thanks for doing this. If I get stuck I will let you know. I know the test better than ever now and fixing them should be quick for me.

[DarqueWarrior]

I am in the process of writing tests for the new classes and making sure all the existing tests pass. I am not sure it will be safe for me to push my changes back to your master branch. I will figure out the best way to get the changes in to the master branch on the original repo. I really appreciate this PR I really like the attribute approach.

[DarqueWarrior]

I am also going to replace all the dynamic parameters with this method. I noticed some functions still have dynamic parameters.

[DarqueWarrior]

You might want to be careful with code like this. If Get-VSTeamBuild returns an empty Array this will post the following error.

The property 'name' cannot be found on this object. Verify that the property exists.

[DarqueWarrior]

I changed the code to this.

foreach ($b in (Get-VSTeamBuild -ProjectName $(_getDefaultProject))) {
            if ($b.name -like "*$WordToComplete*") {
               $results.Add([CompletionResult]::new($b.name))
            }

[DarqueWarrior]

Funny that the code works because builds don't have a name. They have a buildNumber. But that code was working... I changed to buildNumber and it is still working.

[jhoneill]

It might be to do with strictmode . When it's off ($null).name is fine, , when it is on it gives that error. I noticed the tests turn strictmode on, and I have a tendency to write code which relies on it being off. If you need/want to change that anywhere feel free to do so :-)

There are a couple of cases where dynamic parameters may still be the most sensible way to do it - that's usually where the value of parameter A determines if parameter B is visible or not. And I may have left some in that are only used once.

[DarqueWarrior]

Got it! Good to know.

[SebastianSchuetze]

For reference, PR #289 is handling code parts that are merged and others that need improvements.

[DarqueWarrior]

These changes where merged via my Review PR.