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

refactor(scoop-list): Allow list manipulation #4718

Merged
merged 3 commits into from
Feb 8, 2022
Merged

Conversation

rashil2000
Copy link
Member

@rashil2000 rashil2000 commented Feb 7, 2022

Description

Return PSObject instead of printing to terminal.

Motivation and Context

Allows manipulation, sorting etc. of the list with the help of PowerShell's pipelines.

Closes #3162
Closes #4138
Closes #4179

How Has This Been Tested?

Before

image

image

After

image

image

More features (not possible before)

Homebrew-style output:

image

Filter apps from a bucket:

image

Checklist:

  • I have read the Contributing Guide.
  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

@rashil2000
Copy link
Member Author

In the next PR I will add lastwritetime field so that we can sort the list by last updated/installed time

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

Source or Bucket? I think Bucket is better for Scoop.

@rashil2000
Copy link
Member Author

Source because it can be a local file path or a remote URL also, so Bucket won't fit there

@rashil2000
Copy link
Member Author

I just realized that this PR (and the previous one for buckets by @HUMORCE) does not work in Cmd and Bash. No output is shown, unless we remove the exit $lastexitcode part from these lines:

" pwsh -noprofile -ex unrestricted -command `"& '$resolved_path' $arg %args%;exit `$lastexitcode`"",

" pwsh.exe -noprofile -ex unrestricted -command `"& '$resolved_path' $arg $@;exit \`$lastexitcode`"",

@niheaven any idea why these are essential, and whether we can remove them?

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

This line has been added here for a long time and I must check original codes.

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

Found an old explanation of this exit $LastExitCode (#339 (comment)), Maybe it's better to find out why there is no output...

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

image

Some suggestions: For this generated manifest, tell user this is Generated and a manifest location is more better. This needs further parsing for manifest location with workspace.

@rashil2000
Copy link
Member Author

Found an old explanation of this exit $LastExitCode (#339 (comment)), Maybe it's better to find out why there is no output...

I kinda agree with nightroman there. I didn't fully understand the explanation luke gave (also there are no examples in his comment to reproduce the problem if we remove it).

Maybe we just put exit %errorlevel% at the end of the .cmd file?

@rashil2000
Copy link
Member Author

I just checked. If you run the command powershell -nop -c "exit 1" in Cmd, it will show the correct exitcode. The exit $LastExitCode in the line is redundant IMO.

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

See this (PS7) and this (PS5)

The process exit code is determined by status of the last (executed) command within the script block. The exit code is 0 when $? is $true or 1 when $? is $false. If the last command is an external program or a PowerShell script that explicitly sets an exit code other than 0 or 1, that exit code is converted to 1 for process exit code. To preserve the specific exit code, add exit $LASTEXITCODE to your command string or script block.

And in @nightroman's repo, we may use $Global:LASTEXITCODE in shim: https://github.com/nightroman/PowerShellTraps/tree/master/Basic/LastExitCode

@rashil2000
Copy link
Member Author

rashil2000 commented Feb 7, 2022

Instead of using -Command parameter, we can use -File, which does not suffer from the exitcode limitation described in the Microsoft Docs. Any exit code is returned to the Cmd shell as is.

powershell -noprofile -ex unrestricted -file "D:\Data\Scoop\apps\scoop\current\bin\scoop.ps1"  %args%

(I'm not even sure why we use -Command in the first place, when -File parameter exists exactly for this purpose).

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

Instead of using -Command parameter, we can use -File, which does not suffer from the exitcode limitation described in the Microsoft Docs. Any exit code is returned to the Cmd shell as is.

powershell -noprofile -ex unrestricted -file "D:\Data\Scoop\apps\scoop\current\bin\scoop.ps1"  %args%

(I'm not even sure why we use -Command in the first place, when -File parameter exists exactly for this purpose).

PS5 doesn't document exit code for -File clearly, and this code block is written about 8 years ago while PS's version is 2. So I'm not sure if it will output correct exit code in current PowerShell edition (5.1 and 7).

If -File works, it is better, IMO.

@rashil2000
Copy link
Member Author

I tested in PS 5.1 before.

Just now, I tested in PS 7.1.2.

Both work nicely 👍

@niheaven
Copy link
Member

niheaven commented Feb 7, 2022

Not only scoop itself, but also other apps with ps1 shim? If so, let's use -File and test in develop branch.

Emm, it's better to open a new PR for such changes.

@HUMORCE
Copy link
Member

HUMORCE commented Feb 8, 2022

I just realized that this PR (and the previous one for buckets by @HUMORCE) does not work in Cmd and Bash. No output is shown, unless we remove the exit $lastexitcode part from these lines

switch parameter for return? nothing else comes to mind at this time.

@rashil2000
Copy link
Member Author

I just realized that this PR (and the previous one for buckets by @HUMORCE) does not work in Cmd and Bash. No output is shown, unless we remove the exit $lastexitcode part from these lines

switch parameter for return? nothing else comes to mind at this time.

It will work once #4721 is merged

@HUMORCE
Copy link
Member

HUMORCE commented Feb 8, 2022

It will work once #4721 is merged

i seem to have missed some comments in this thread on gh mobile.

@niheaven
Copy link
Member

niheaven commented Feb 8, 2022

image

Some suggestions: For this generated manifest, tell user this is Generated and a manifest location is more better. This needs further parsing for manifest location with workspace.

What about this? Or maybe another PR?

@rashil2000
Copy link
Member Author

I have not added any new features in this PR. Scoop list shows the same information as it did before.

Each new feature should be a new PR IMO

@rashil2000 rashil2000 merged commit e450843 into develop Feb 8, 2022
@rashil2000 rashil2000 deleted the scoop-list branch February 8, 2022 11:56
se35710 pushed a commit to se35710/scoop that referenced this pull request Mar 8, 2022
* refactor(scoop-list): Allow list manipulation

* update changelog
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.

4 participants