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(search): Improve search output and ratelimit handling #3446

Closed
wants to merge 16 commits into from

Conversation

r15ch13
Copy link
Member

@r15ch13 r15ch13 commented May 9, 2019

  • Now displays the description (local bucket only).
  • Use Invoke-RestMethod
  • Show messages if GitHub rate limit is reached
  • Handle rate limit on currently running query (using X-RateLimit-Remaining returned by GitHub, PowerShell 6 only)
  • Improved binary search (now shows all matches, architectures was ignored)

image

image

image

Related #3853

libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
@r15ch13
Copy link
Member Author

r15ch13 commented Jun 1, 2019

The shortcut searching was inspired by ScoopInstaller/Extras#2227 because @ssfjhh couldn't find Process Explorer. The exe is called procexp.exe and it can only be found by the shortcut name.

I also improved the output to be more readable:

image

@kidonng kidonng mentioned this pull request Jul 14, 2019
8 tasks
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved

$results = $names | Where-Object { !(test-path $(Find-BucketDirectory $_)) } | ForEach-Object {
@{"bucket" = $_; "results" = (search_remote $_ $query)}
$results = known_buckets | Where-Object { !(test-path $(Find-BucketDirectory $_)) } | ForEach-Object {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for subshell

Suggested change
$results = known_buckets | Where-Object { !(test-path $(Find-BucketDirectory $_)) } | ForEach-Object {
$results = known_buckets | Where-Object { !(Test-Path (Find-BucketDirectory $_)) } | ForEach-Object {

libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
libexec/scoop-search.ps1 Outdated Show resolved Hide resolved
r15ch13 and others added 9 commits October 20, 2019 13:57
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
Co-Authored-By: Jakub Čábera <cabera.jakub@gmail.com>
param(
[Parameter(Mandatory = $true)]
$Query,
[Switch] $Remote
Copy link
Contributor

@Ash258 Ash258 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing documentation for this parameter.

Why is this parameter needed? I would expect that if this is specified ONLY remotes are searched. But locals are searched all the time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query is now mandatory. Change documentation for help command, reflecting this change.

Write-Host 'Searching in local buckets ...'
$local_results = @()

foreach ($bucket in (Get-LocalBucket) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing bracket

Suggested change
foreach ($bucket in (Get-LocalBucket) {
foreach ($bucket in (Get-LocalBucket)) {

Copy link
Contributor

@Ash258 Ash258 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Bottom = before
Up = after

Maybe it could be handled same as binary $exe -> $name

Edit:

image

Same process as for binaries


foreach ($shortcut in $app.shortcuts) {
if($shortcut -is [Array] -and $shortcut.length -ge 2) {
$name = $shortcut[1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$name = $shortcut[1]
$executable = $shortcut[0]
$name = $shortcut[1]

foreach ($shortcut in $app.shortcuts) {
if($shortcut -is [Array] -and $shortcut.length -ge 2) {
$name = $shortcut[1]
if($name -match $query) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if($name -match $query) {
if ($name -match $query -or $executable -match $query) {

foreach ($bucket in (Get-LocalBucket) {
$result = search_bucket $bucket $query
if(!$result) {
return
Copy link
Contributor

@Ash258 Ash258 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect you do not want to break on first bucket, which do not have results

Suggested change
return
continue

@Ash258
Copy link
Contributor

Ash258 commented Oct 29, 2019

Would be possible to move functions into lib\search.ps1 instead of keeping it inside lib-exec?

@Ash258
Copy link
Contributor

Ash258 commented Oct 29, 2019

I think more user-friendly it would be to follow getopt way for parameters.

Now you will get this:

image

You could go rather with optional query param and then manual check if -not $query { error ; break } or with getopt approach

'getopt', 'manifest', 'install', 'versions', 'config' | ForEach-Object {
    . "$PSScriptRoot\..\lib\$_.ps1"
}

$Remote = $false
$opt, $Query, $err = getopt $args 'r' 'remote'

if (-not $Query) { error 'QUERY IS NEEDED'; exit 1 }
if ($opt.r -or $opt.remote) { $Remote = $true }

Comment on lines +38 to 42
try {
$query = New-Object Regex $query, 'IgnoreCase'
} catch {
abort "Invalid regular expression: $($_.exception.innerexception.message)"
}
Copy link
Contributor

@Ash258 Ash258 Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would move this outside function. When it will be standalone inside scoop-search.ps1 you will save abort call and can handle failsafe scenario better.

Then the query, which is passing into search_bucket function could be Regex object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you do not need to create regex for each bucket. This regex is same for all buckets

@rashil2000
Copy link
Member

Superceded by #4997

@rashil2000 rashil2000 closed this Jun 19, 2022
@rashil2000 rashil2000 deleted the feature/improve-search branch June 24, 2022 18:35
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.

3 participants