First drafted client cmdlets from Hackathon 221 #1760

denelon · 2021-12-01T22:13:05Z

Have you signed the Contributor License Agreement?
Are you working against an Issue?

Closes #674

This is the first set of PowerShell cmdlets drafted during the 2021 Hackathon at Microsoft to explore PowerShell syntax for the Windows Package Manager. This is a very early draft and is subject to substantial change. Notes and historical context are in the README.md.

Microsoft Reviewers: Open in CodeFlow

denelon · 2021-12-01T22:24:00Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Enable-WinGetLocalManifest.ps1

@@ -0,0 +1,41 @@
+Function Enable-WinGetLocalManifest


From @JohnMcPMS on earlier PR:

These should probably be something like Set-WinGetAdminSetting -Setting LocalManifest -Value true rather than creating a new cmdlet for every setting.

From @denelon

The PowerShell cmdlet structure tends to use Enable and Disable. It makes them more discoverable. Maybe someone from the community can weigh in on this one. @doctordns what do you think?

From @JohnMcPMS

I just expect the possibility of non-boolean values in the future, but it could also be Enable/Disable for booleans and Set in the future for non-booleans. But the larger takeaway is to make the actual setting that is being modified be a parameter rather than making new cmdlets per setting.

I like Enable and Disable, but I agree that it could probably be simplified to Enable-AdminSetting -Setting LocalManifest

As noted somewhere else - the enable verb is used to move a resource to be available or active. Disable does the opposite. I would prefer Set, especially since the target could possibly take non boolean values.

IMHO Enable-Adminsetting -Setting is not great PowerShell. The noun should be the specific thing we are enableling (eg Enable-LocalUser, Enable-PSRemoting). Enabler-LocalManifest would be better than this.

What other commands are there that have the noun "LocalManifest" ?

I guess I would agree about the noun being specific; However, I still believe that enable and disable are appropriate based on the Microsoft Documentation. From https://docs.microsoft.com/en-us/powershell/scripting/developer/cmdlet/approved-verbs-for-windows-powershell-commands?view=powershell-7.1 -

Disable (d) | Configures a resource to an unavailable or inactive state. For example, the Disable-PSBreakpoint cmdlet makes a breakpoint inactive. This verb is paired with Enable. Enable (e) | Configures a resource to an available or active state. For example, the Enable-PSBreakpoint cmdlet makes a breakpoint active. This verb is paired with Disable.

In this case, it is making the setting active or inactive. A setting can be considered a resource just as much as a breakpoint can be considered a resource

.github/actions/spelling/expect.txt

Trenly

Didn't get through all the files, but here's what I noticed so far

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Add-WinGetSource.ps1

Trenly · 2021-12-01T22:47:57Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Add-WinGetSource.ps1

+    }
+}
+
+Export-ModuleMember -Function Add-WinGetSource


Is there a reason these are being exported from here rather than being included in the FunctionsToExport section of the module manifest? It may be better to have one or the other, rather than both

Trenly · 2021-12-01T22:51:25Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Enable-WinGetLocalManifest.ps1

@@ -0,0 +1,41 @@
+Function Enable-WinGetLocalManifest


I like Enable and Disable, but I agree that it could probably be simplified to Enable-AdminSetting -Setting LocalManifest

Trenly · 2021-12-01T22:53:45Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Find-WinGetPackage.ps1

+        This example searches for a package containing "Package" as a valid name on all configured sources.
+    #>
+    PARAM(
+        [Parameter(Position=0)] $Filter,


Do these need to be explicitly stated as strings? What happens if I try to pipe an object or an array to the function?

This is very early. Pipelining won't work well with anything depending on list or a display where we truncate strings in the Terminal until we get JSON output via the COM API.

Fair point; I guess I’m thinking like a tester in that someone could do something that would end up being like -

Install-WingetPackage -Id @{“Id”=“Google.Chrome”}

I know its not something that would be common, but if people try writing custom scripts, what happens if an error like that occurrs? I’m assuming it will be fine as is, but just want to be sure that we are aware of any potential adverse effects

doctordns · 2021-12-02T10:53:44Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Get-WinGetVersion.ps1

+                $Result = [version](& "winget" $WinGetArgs).trimstart("v")
+        }
+        catch {
+        }


Good catch, so to speak.
Shoudl the catch throw an exception or just issue a warning?

I don't think it needs to throw since it is set to "0.0.0.0" beforehand and shouldn't cause any issues. Maybe it doesn't need a warning either, perhaps a Write-Verbose is fine. Just as long as it is being considered and if the block is empty that it's intentional

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Install-WinGetPackage.ps1

…inGetPackage.ps1 Co-authored-by: Kaleb Luedtke <[email protected]>

Trenly · 2021-12-02T04:32:54Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Enable-WinGetLocalManifest.ps1

+    PARAM {
+        [Parameter()] [switch]                  $VerboseLog
+    }


Suggested change

PARAM {

[Parameter()] [switch] $VerboseLog

}

PARAM ( )

Braces mistakenly used instead of Parens; Verbose doesn't appear to be valid for settings?

Should this be just -Verbose?

I think that -Verbose is an inherent property if it has a cmdlet binding, and since the settings command for winget doesn't support a verbose option, I don't think it needs to be included here at all

-Verbose is a standard parameter, so if the intent of this is just to create verbose user output, then it need not be a separate parameter here.

At least with normal functions, you enable this by making the function an advanced function. Just add [CMDLETBINDING] just above the parm block.

I initially thought this switch might be a special diag log for winget? Useful to clarify that.

I think it is intended to be a diagnostic log for winget, but I don't think the setting command supports the diagnostic logging in winget. It's also probably not important to have a cmdlet binding here, since there is nothing using Write-verbose. Since these functions are just a wrapper for winget commands anyways, I would argue that keeping it simple at the start is better. If the verbose functionality is wanted or needed, then it can be added later

Trenly · 2021-12-02T04:38:49Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Edit-WinGetClientSetting.ps1

+    BEGIN
+    {
+        [string[]] $WinGetArgs  = "Settings"
+        $WinGetArgs += "--Verbose-Logs", $VerboseLog


Suggested change

$WinGetArgs += "--Verbose-Logs", $VerboseLog

I get an error if verbose logs is specified here; Verbose isn't listed in the help for the winget settings command

It is a valid parameter for the client. It's just not listed in help yet.

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Enable-WinGetLocalManifest.ps1

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Disable-WinGetLocalManifest.ps1

Trenly · 2021-12-02T05:17:52Z

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Invoke-WinGetCommand.ps1

+
+                foreach($item in $Index) {
+                    if($Item.Ends) {
+                        #try {


Suggested change

#try {

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Uninstall-WinGetPackage.ps1

Trenly · 2021-12-02T05:51:51Z

tools/PowerShell/README.md

+
+We started this project as an exploration to design the right set of cmdlets with PowerShell approved nouns and verbs. As we began to explore wrapping the Windows Package Manager we identified several areas of complexity. Rather than slow down progress on building PowerShell cmdlets, we decided to forge ahead and call out the areas where the experience with pipelines is sub-optimal.
+
+For example, the Windows Package Manager (later referenced as just CLI) CLI was designed for displaying output in the standard width Windows Terminal. As such, long package names and "Id"s are truncated with a single width ellipsis character. Users wanting to use the PowerShell pipeline to pass values to another cmdlet will likely encounter undesired behavior due to this truncation. We have decided to declare the "Microsoft.WinGet" module as an alpha release until these problems have been resolved. We are planning to change the status of "PreRelease" to beta once we believe we have addressed the issues related to piping the output to other cmdlets.


Suggested change

For example, the Windows Package Manager (later referenced as just CLI) CLI was designed for displaying output in the standard width Windows Terminal. As such, long package names and "Id"s are truncated with a single width ellipsis character. Users wanting to use the PowerShell pipeline to pass values to another cmdlet will likely encounter undesired behavior due to this truncation. We have decided to declare the "Microsoft.WinGet" module as an alpha release until these problems have been resolved. We are planning to change the status of "PreRelease" to beta once we believe we have addressed the issues related to piping the output to other cmdlets.

For example, the Windows Package Manager (later referenced as winget or CLI) CLI was designed for displaying output in the standard width Windows Terminal. As such, long package names and "Id"s are truncated with a single width ellipsis character. Users wanting to use the PowerShell pipeline to pass values to another cmdlet will likely encounter undesired behavior due to this truncation. We have decided to declare the "Microsoft.WinGet" module as an alpha release until these problems have been resolved. We are planning to change the status of "PreRelease" to beta once we believe we have addressed the issues related to piping the output to other cmdlets.

This is, at one level, very poor. You have a property with a value that you then truncate. I really hate any application that can not round-trip data - I set a string but you return it to me different to what was set.

One solution might be to have an -JSON switch that just emits the information in JSON. Or have a -RAW switch that does no formatting?

And if it were me, I;'d fix this before calling it alpha.

I completely agree with the feedback here.

We're going to add the capability to output data as JSON in the COM API. Then we will switch these cmdlets to use that mechanism. These cmdlets are very "Hackathon". My thought is that a (mostly) "working" sample at least gets the nouns and verbs out in a manner that we can share to get feedback.

I don't know what to call something that's earlier than "alpha". There seemed to be some limitations on what we could use for pre-release values.

tools/PowerShell/README.md

doctordns · 2021-12-02T14:51:05Z

Is there an easy way to download these cmdlets to test them??
I am not sure where to find these in github! ;-)

Trenly · 2021-12-02T15:02:21Z

Is there an easy way to download these cmdlets to test them?? I am not sure where to find these in github! ;-)

Clone the repository, checkout the branch associated with this PR, copy the path to the ,psm1 file, then Import-Module <path-to-psm1>

Suggestions1760

denelon · 2021-12-02T17:48:29Z

@doctordns

Is there an easy way to download these cmdlets to test them?? I am not sure where to find these in github! ;-)

Not yet. We need to get some tests in place and look at a build to generate them for release. This was very much a Hackathon project. I consider myself a noob at PowerShell. I just knew that we had to get this going sooner rather than later. As soon as we meet the requirements for signing, we will get them signed and then publish to the PowerShell gallery. It involves other teams performing reviews and checks, so that's not likely before next year. My apologies for this taking so long.

Co-authored-by: Kaleb Luedtke <[email protected]>

doctordns · 2021-12-02T17:55:46Z

At least, can you zip up the module and leave it here on GitHub? Let's get the basics right before pushing to the PS Gallery. These cmdlets are an excellent start, but need a lot of basic work before, IMHO, they are ready to push to a wider audience.

And it may just be me, but I am having a huge problem trying to work out what is where! :-) I almost need a Readme(FIRST).MD to tell me where to look for what.

Treat empty strings in constructors as null

tools/PowerShell/Microsoft.WinGet.Client/crescendo/WindowsPackageManager.psd1

…ageManager.psd1

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Add-WinGetSource.ps1

tools/PowerShell/Microsoft.WinGet.Client/src/Microsoft.WinGet.Client.psm1

adjusted example text

denelon added 3 commits December 1, 2021 09:43

First drafted client cmdlets from Hackathon 221

3891c07

spelling

cf4659b

spelling

897514e

ghost added the Issue-Feature This is a feature request for the Windows Package Manager client. label Dec 1, 2021

denelon mentioned this pull request Dec 1, 2021

First drafted client cmdlets from Hackathon 221 #1757

Closed

2 tasks

fixed filename

ea12017

denelon commented Dec 1, 2021

View reviewed changes

.github/actions/spelling/expect.txt Show resolved Hide resolved

denelon requested a review from JohnMcPMS December 1, 2021 22:26

Trenly reviewed Dec 1, 2021

View reviewed changes

denelon and others added 2 commits December 1, 2021 16:11

Update tools/PowerShell/Microsoft.WinGet.Client/src/Library/Install-W…

ca7b764

…inGetPackage.ps1 Co-authored-by: Kaleb Luedtke <[email protected]>

Update tools/PowerShell/Microsoft.WinGet.Client/src/Library/Install-W…

9793fae

…inGetPackage.ps1 Co-authored-by: Kaleb Luedtke <[email protected]>

Trenly reviewed Dec 2, 2021

View reviewed changes

Trenly and others added 3 commits December 2, 2021 10:43

Suggested changes from 1760

3510511

Apply Additional Suggestions

92b3954

Merge pull request #1 from Trenly/Suggestions1760

6114daf

Suggestions1760

Update tools/PowerShell/README.md

dcc2103

Co-authored-by: Kaleb Luedtke <[email protected]>

denelon mentioned this pull request Dec 2, 2021

PowerShell cmdlets #1407

Closed

2 tasks

Treat empty strings in constructors as null

29af10e

ethanbergstrom mentioned this pull request Dec 4, 2021

Add Native PowerShell Support #221

Closed

Merge pull request #2 from Trenly/ClassEnhancements

9c6025d

Treat empty strings in constructors as null

denelon marked this pull request as ready for review December 10, 2021 20:06

denelon requested a review from a team as a code owner December 10, 2021 20:06

florelis reviewed Dec 10, 2021

View reviewed changes

tools/PowerShell/Microsoft.WinGet.Client/crescendo/WindowsPackageManager.psd1 Outdated Show resolved Hide resolved

Update WindowsPackageManager.psd1

baf0302

denelon commented Dec 10, 2021

View reviewed changes

tools/PowerShell/Microsoft.WinGet.Client/crescendo/WindowsPackageManager.psd1 Outdated Show resolved Hide resolved

Update tools/PowerShell/Microsoft.WinGet.Client/crescendo/WindowsPack…

d0806f7

…ageManager.psd1

msftrubengu reviewed Dec 10, 2021

View reviewed changes

tools/PowerShell/Microsoft.WinGet.Client/src/Library/Add-WinGetSource.ps1 Outdated Show resolved Hide resolved

msftrubengu reviewed Dec 10, 2021

View reviewed changes

tools/PowerShell/Microsoft.WinGet.Client/src/Microsoft.WinGet.Client.psm1 Outdated Show resolved Hide resolved

denelon added 6 commits December 10, 2021 15:08

Merge branch 'microsoft:master' into cmdlets

0785126

implemented uninstall

0da9373

adjusted example text

Update Uninstall-WinGetPackage.ps1

e3a96c1

tabs vs. spaces

1cf5cf0

removing commented line

a060d74

removed extra brace

adff0a9

msftrubengu approved these changes Dec 11, 2021

View reviewed changes

denelon merged commit c6650aa into microsoft:master Dec 13, 2021

denelon deleted the cmdlets branch December 13, 2021 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First drafted client cmdlets from Hackathon 221 #1760

First drafted client cmdlets from Hackathon 221 #1760

denelon commented Dec 1, 2021 •

edited

Loading

denelon Dec 1, 2021

Trenly Dec 1, 2021

doctordns Dec 2, 2021

Trenly Dec 2, 2021 •

edited

Loading

Trenly left a comment

Trenly Dec 1, 2021

Trenly Dec 1, 2021

Trenly Dec 1, 2021

denelon Dec 2, 2021

Trenly Dec 2, 2021

This comment was marked as resolved.

doctordns Dec 2, 2021

Trenly Dec 2, 2021 •

edited

Loading

Trenly Dec 2, 2021

doctordns Dec 2, 2021

Trenly Dec 2, 2021

doctordns Dec 2, 2021

Trenly Dec 2, 2021

Trenly Dec 2, 2021

denelon Dec 2, 2021

Trenly Dec 2, 2021

Trenly Dec 2, 2021

doctordns Dec 2, 2021

denelon Dec 2, 2021

doctordns commented Dec 2, 2021

Trenly commented Dec 2, 2021

denelon commented Dec 2, 2021

doctordns commented Dec 2, 2021


		We started this project as an exploration to design the right set of cmdlets with PowerShell approved nouns and verbs. As we began to explore wrapping the Windows Package Manager we identified several areas of complexity. Rather than slow down progress on building PowerShell cmdlets, we decided to forge ahead and call out the areas where the experience with pipelines is sub-optimal.

		For example, the Windows Package Manager (later referenced as just CLI) CLI was designed for displaying output in the standard width Windows Terminal. As such, long package names and "Id"s are truncated with a single width ellipsis character. Users wanting to use the PowerShell pipeline to pass values to another cmdlet will likely encounter undesired behavior due to this truncation. We have decided to declare the "Microsoft.WinGet" module as an alpha release until these problems have been resolved. We are planning to change the status of "PreRelease" to beta once we believe we have addressed the issues related to piping the output to other cmdlets.

First drafted client cmdlets from Hackathon 221 #1760

First drafted client cmdlets from Hackathon 221 #1760

Conversation

denelon commented Dec 1, 2021 • edited Loading

Closes #674

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trenly Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Trenly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Trenly Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doctordns commented Dec 2, 2021

Trenly commented Dec 2, 2021

denelon commented Dec 2, 2021

doctordns commented Dec 2, 2021

denelon commented Dec 1, 2021 •

edited

Loading

Trenly Dec 2, 2021 •

edited

Loading

Trenly Dec 2, 2021 •

edited

Loading