jhoneill commented 4 years ago

PR Summary

Updates to completers , validators and their associated caches Detailed in changelog.md.

ProcessCache, ProjectCache, and the WorkItemTypeCache now have get, update and invalidate methods so they can be used without referring to their internals.
Their associated completers and validators use these methods.
_getWorkItemTypes has been improved
Completers now wrap items which need to be quoted in quote marks.
Validators do not reject null or empty (that is the job of other parameter attributes)
VSTeamWorkItemTypeCache was missing from _classes. Classes should build to the PSM1 and not to a dot sourced ps1 file (dot sourcing makes classes invisible outside the module which defines them and causes the shortening of parameter attributes (removing 'attribute' from the name) to fail.

PR Checklist

[ ] [Write Help] No changes to user functionality
[ ] [Write Unit Test] Existing tests should still work
[ X] [Update CHANGELOG.md] Yes. Although most of the detail can be removed before publishing.

SebastianSchuetze commented 4 years ago

@jhoneill did you notice that all your PRs fail with the same error message on all environments? I find this strange since I didn't have that problem.

@DarqueWarrior is this coming from changes from us?

UPDATE: Problem is explained in #315

jhoneill commented 4 years ago

Your PR builds are not running as I mention in the other one. Due to a wrong change you did:

jhoneill@9feee71#diff-a9c43fc4a14707ca2c3bb16664e4031f

you changed the property outputFile to "vsteam.psm1" not sure why you did it, but that is the wrong place. ;-)

Please run the build script locally before committing, you would have caught it like I did. :-)

Now, some thin skinned people would get offended at the suggestion they didn't build code before making a PR. :-) I don't think you can have read the change log, which explains that your build and my build are different, why and the choice you need to make It references a issue over on the PowerShell repo which advises Do not put classes in PS1 files and Dot source them, and don't put them in "modules to load" in the manifest either (which was my work round). To be able to reference a class outside the module it comes from it must be in the PSM1 file. In a PS1 dot sourced from the PS1 will fail. I think I changed it when I provided the completers and validators but @DarqueWarrior undid that change (again, I think) so the code in your master branch is still using the dot sourcing method.

Previously you were unhappy that I changed too much so I didn't want to check in a change to your build process and gave you a choice, either change 1 line in each of two of your files to load the classes "properly" PSM, or undo one of my changes to keep dot sourcing.

I've changed the build files and it builds on Mac and Windows but the Linux build dies and there is no way to get the files that it built to see why. There are some pester tests which don't pass but Azure devops won't show me the test summary, so I can't see which of tests have an issue. It may the old problem of tests relying on loading specific PS1 files rather than loading the module. Following past advice I haven't even tried to run the unit tests locally.

SebastianSchuetze commented 4 years ago

I didn't want to offend you. I just didn't understand the implications. As we are very cautious about changes in the core functionality we need to fully understand that the change for inclusion of the classes is a must-have.

I have read your issue on the PowerShell repo. Does it have to do only with the completer classes? Why do classes have to be accessible from outside of the module? Could you shortly explain?

I am not against best practices, but also need to trust that core functionalities are changing with the right quality and intend of the module and @DarqueWarrior.

jhoneill commented 4 years ago

Now, some thin skinned people would get offended ...

I didn't want to offend you.

I should have put a :-) I'm not one of those thin skinned people, and no offence was taken . We are all trying very hard not to offend anybody (I think) and that might be having other side effects. Don't worry; I have the skin of a Rhino and I assume shared good will on github.

we are very cautious about changes in the core functionality ... ... I am not against best practices,

Yes, rightly so, and fully understood.
There is also the case where you have a big project, which this is, doing something just a little less than best but which is not truly bad and the maintainer has to ask "Is the improvement worth the work needed". For example there's a "community best practice" to use multiples of four spaces for indents, and this project uses multiples of three. But telling you to change from three to four would improve nothing for end users, and need a tonne of work, so leaving things as they are makes the most sense (and IMHO it's rude to ask people to change things are matters of style). On the other hand I asked you to take out the Set-StrictMode because it was a small task to follow a best practice of "Neither rely on nor break with any user setting of strict mode" and it had a clear benefit.

Spacing is a "You might ,if you want..." change, Strictmode is a "Really, you must..." change and where you put classes is a "You should" change, it's better if you do, but nothing breaks if you don't. So it is your call but I need to explain it, to which leads to...

I have read your issue on the PowerShell repo. Does it have to do only with the completer classes? > Why do classes have to be accessible from outside of the module? Could you shortly explain?

It is most visible with parameter attribute classes but it applies to all classes. Some things work OK with the classes private, for example [VSTeamProjectPermissions] is OK as the class for a parameter value, and tab completion cycles round all the different values. But using [VSTeamProjectPermissions]::BYPASS_RULES gives an error.

The Microsoft docs say you can write a class with the name ValidateProcessAttribute And then write :

[Parameter(Mandatory , Position = 1 )]
[ValidateProcess()]
$ProcessName

Omitting "Attribute" when you use the class. This fails if the class is Dot Sourced. TBH I have stopped writing the name without the attribute part - it breaks in too many cases.

Here are some examples of what fails with the dot sourcing method.

#501 PS7 C:\Users\ME\Documents\github\vsteam>git checkout master

Already on 'master'
Your branch is up to date with 'origin/master'.
#502 PS7 C:\Users\ME\Documents\github\vsteam>.\Build-Module.ps1

Processing: ... ... Publish complete to C:\Users\ME\Documents\github\vsteam\dist

#503 PS7 C:\Users\ME\Documents\github\vsteam>using module .\dist\VSTeam.psd1 ; Set-VSTeamAccount -Account <<my org>> -PersonalAccessToken <mytoken> ; 
Set-VSTeamDefaultProject -Project '<<my org>> - CRM'

#504 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamVersions]::Account

InvalidOperation: Unable to find type [VSTeamVersions].

#505 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamProjectCache]::timestamp

InvalidOperation: Unable to find type [VSTeamProjectCache].

#506 PS7 C:\Users\ME\Documents\github\vsteam>$p = new-object projectvalidateattribute

New-Object: Cannot find type [projectvalidateattribute]: verify that the assembly containing this type is loaded.

These were painful writing "Add-VsTeamProcess", because the parameters could not use the attributes which were private to the module, and an Add operation must either update the cache or invalidate it. During development Add-VsTeamProcess wasn't part of the module, it feels right to write a function & ensure it works before making it part of the module, without loading dependencies (and their dependencies) from the source tree during development.

Here's what happens if the classes are loaded from the PSM1

#501 PS7 C:\Users\ME\Documents\github\vsteam>git checkout ProcessManagement

Switched to branch 'ProcessManagement'
Your branch is up to date with 'origin/ProcessManagement'.
#502 PS7 C:\Users\ME\Documents\github\vsteam>.\Build-Module.ps1 

Processing: ... ...Publish complete to C:\Users\ME\Documents\github\vsteam\dist
#503 PS7 C:\Users\ME\Documents\github\vsteam>using module .\dist\VSTeam.psd1 ; Set-VSTeamAccount -Account   <<my Org>> -PersonalAccessToken <<my token>> ; Set-VSTeamDefaultProject -Project '<<my Org>> - CRM'

#504 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamVersions]::Account

https://dev.azure.com/<<my Org>>

#505 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamProjectCache]::timestamp

-1

#506 PS7 C:\Users\ME\Documents\github\vsteam>$p = new-object projectvalidateattribute
#507 PS7 C:\Users\ME\Documents\github\vsteam>$p.Validate("foo",$null)                

MetadataError: C:\Users\ME\Documents\github\vsteam\dist\VSTeam.psm1:466:10
Line |
 466 |           throw [ValidationMetadataException]::new(
     |           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     | 'foo' is not a valid project. Valid projects are: '<<my Org>> - Interfaces', '<<my Org>> - Infrastructure and Environment', '<<my Org>> - DevOps', '<<my Org>> - UAT', 'Reports', '<<my Org>> - CRM', '<<my Org>> - Data
     | Migration', '<<my Org>> Change and Observations', '<<my Org>> - Reporting and BI'

#508 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamProjectCache]::timestamp

36

#509 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamProjectCache]::projects 

<<my Org>> - Interfaces
<<my Org>> - Infrastructure and Environment
<<my Org>> - DevOps
<<my Org>> - UAT
Reports
<<my Org>> - CRM
<<my Org>> - Data Migration
<<my Org>> Change and Observations
<<my Org>> - Reporting and BI

#511 PS7 C:\Users\ME\Documents\github\vsteam>[VSTeamProjectPermissions]::GENERIC_READ

The last one tab completes and I can see the [VSTeamProjectPermissions]::GENERIC_READ.value__ returns 1 and so on.

Currently there is a vsteamprocess class, changing the declaration of Get-VSTeamProcess to this :

function Get-VSTeamProcess {
   [CmdletBinding(DefaultParameterSetName = 'List')]
   [OutputType([vsteamprocess])]
...
}

allows a line like this Get-VSTeamProcess | ft to tab-complete name, description ID and so on but only if the vsteamprocess class is in scope for the tab completer.

jhoneill commented 4 years ago

Well at least it is now up-to-date with master, and all the tests pass on Windows and MacOS. still don't know what the issue is with linux

DarqueWarrior commented 4 years ago

Thanks for taking the time to explain the changes.

I don't want any classes or properties of classes exposed outside the module. Therefore, work might need to be done to abstract all that behind functions. Classes were originally introduced to support the use of ShiPS for the provider. Their use as expanded but the desire to expose them has not changed.

I don't want to change the build process at this time.

jhoneill commented 4 years ago

Thanks for taking the time to explain the changes.

I don't want any classes or properties of classes exposed outside the module. Therefore, work might need to be done to abstract all that behind functions. Classes were originally introduced to support the use of ShiPS for the provider. Their use as expanded but the desire to expose them has not changed.

I don't want to change the build process at this time.

OK, understood. You realise that if you expose , (e.g). VSTeamProcess outside the module you specify, [OutputType([Vsteam])] in Get- Set- Add- VSTeamProcess then intellisense will fill in properties when you do (e.g) Get-VSteamProcess | format-table. It also means you can't develop a function which uses one of the existing validators or completers without integrating it into the module and rebuilding and restarting PowerShell (because the module seems unhappy re-loading) for each iteration.

DarqueWarrior commented 4 years ago

I moved the changes for the spaces to another PR that has been merged already. All the values are strings so there is no reason not to just quote all the results.

DarqueWarrior commented 4 years ago

You realise that if you expose , (e.g). VSTeamProcess outside the module you specify, [OutputType([Vsteam])] in Get- Set- Add- VSTeamProcess then intellisense will fill in properties when you do (e.g) Get-VSteamProcess | format-table.

We do return some classes from Functions that is true. But at this point what problem is that causing? I don't want to change our build process to fix a problem we don't have.

DarqueWarrior commented 4 years ago

It also means you can't develop a function which uses one of the existing validators or completers without integrating it into the module and rebuilding and restarting PowerShell (because the module seems unhappy re-loading) for each iteration.

I have experienced this and it is a bit annoying. But not so much so that I want to change the core build at this time. As a contributor it is a pain but as a user it is a nonissue.

jhoneill commented 4 years ago

It also means you can't develop a function which uses one of the existing validators or completers without integrating it into the module and rebuilding and restarting PowerShell (because the module seems unhappy re-loading) for each iteration.

I have experienced this and it is a bit annoying. But not so much so that I want to change the core build at this time. As a contributor it is a pain but as a user it is a nonissue.

Agreed and understood. The developer workround is OK, and if build changes feel wrong, park them, at least for the time being

You realise that if you expose , (e.g). VSTeamProcess outside the module you specify, [OutputType([Vsteam])] in Get- Set- Add- VSTeamProcess then intellisense will fill in properties when you do (e.g) Get-VSteamProcess | format-table.

We do return some classes from Functions that is true. But at this point what problem is that causing? I don't want to change our build process to fix a problem we don't have.

Well, the absence of unhappy users tells you all you need to know :-) There is a chance to make things better in the future if you make the change but no imperative to do it right now.

I moved the changes for the spaces to another PR that has been merged already. All the values are strings so there is no reason not to just quote all the results.

I saw. In other places PowerShell leaves the quotes out when they are not needed so I tried to follow that but not 100% necessary, just tidy.

MethodsAndPractices / vsteam

Update Completer/Validator classes for #299, cache and class accessibility improvements #313

PR Summary

PR Checklist