Skip to content

refactor(bucket): Move 'Find-Manifest' and 'list_buckets' to 'buckets'#4814

Merged
niheaven merged 5 commits intodevelopfrom
feature/refactor-find-manifest
Mar 23, 2022
Merged

refactor(bucket): Move 'Find-Manifest' and 'list_buckets' to 'buckets'#4814
niheaven merged 5 commits intodevelopfrom
feature/refactor-find-manifest

Conversation

@niheaven
Copy link
Copy Markdown
Member

@niheaven niheaven commented Mar 15, 2022

Description

find_manifest() and Find-Manifest() (former locate()) have similar functionality and now be merged into Find-Manifest().

Also move list_buckets() from libexec\scoop-bucket.ps1 to lib\buckets.ps1.

Motivation and Context

Discussed with @rashil2000

How Has This Been Tested?

scoop bucket and `scoop

Checklist:

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

@rashil2000
Copy link
Copy Markdown
Member

Nice change!

Since we're refactoring bucket methods, can we also incorporate #3348? This will allow us to close #2123

@niheaven
Copy link
Copy Markdown
Member Author

Okay, wait a moment.

@niheaven
Copy link
Copy Markdown
Member Author

Done. Now check for duplicated buckets.

Comment thread lib/buckets.ps1
@rashil2000
Copy link
Copy Markdown
Member

image

The bucket-add function should stop at the first error (ERROR github.com/ScoopInstaller/Extras is not a valid Git URL!), no?

@niheaven
Copy link
Copy Markdown
Member Author

niheaven commented Mar 20, 2022

So you mean we should assume default protocol? The error you met is no protocol provided (it is http? https? ftp? ftps? ssh?)

Ref is here: https://git-scm.com/docs/git-clone#_git_urls

@rashil2000
Copy link
Copy Markdown
Member

I meant that we're showing the error twice. When we get an error (any error) from the Convert-RepositoryUri function, we should stop the add_bucket function (instead of proceeding to the Checking repo... step)

@niheaven
Copy link
Copy Markdown
Member Author

Oh, sorry, haven't noticed that, will change it later.

@niheaven niheaven requested a review from rashil2000 March 21, 2022 02:19
Comment thread lib/buckets.ps1 Outdated
@rashil2000
Copy link
Copy Markdown
Member

Since the location of Find-Manifest has changed, has the import of this function in all other source files been taken care of?

@niheaven
Copy link
Copy Markdown
Member Author

Since the location of Find-Manifest has changed, has the import of this function in all other source files been taken care of?

buckets.ps1 is imported in every lib-exec scripts, so it's not a problem.

@niheaven niheaven merged commit 32de4c5 into develop Mar 23, 2022
@niheaven niheaven deleted the feature/refactor-find-manifest branch March 23, 2022 02:47
@lorengordon
Copy link
Copy Markdown

Traced this patch as the cause of this issue #4917

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