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

fix(scoop-import): Use foreach instead of ForEach-Object for nullity check #5034

Merged
merged 7 commits into from
Jul 7, 2022

Conversation

zStruCat
Copy link
Contributor

@zStruCat zStruCat commented Jul 3, 2022

Description

Check whether the wanted property of bucket is null.
Check if the to-be-imported bucket is already added locally

Context

Closes #5033

@rashil2000
Copy link
Member

I've made some tweaks:

  • Using foreach loop in place of ForEach-Object automatically takes care of null values
  • The add_bucket function has a robust check for duplicate bucket names and origin URIs, so we don't need to add another check. Moreover, if a bucket already exists, we want to show an error while importing, so that users know that bucket is there.

@rashil2000 rashil2000 requested a review from niheaven July 6, 2022 06:24
@rashil2000 rashil2000 changed the title fix(scoop-import) Add nullity check and is in-already-in-local-bucket check fix(scoop-import): Use foreach instead of ForEach-Object for nullity check Jul 6, 2022
niheaven
niheaven previously approved these changes Jul 6, 2022
Copy link
Member

@niheaven niheaven left a comment

Choose a reason for hiding this comment

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

LGTM

@zStruCat
Copy link
Contributor Author

zStruCat commented Jul 6, 2022

The add_bucket function has a robust check for duplicate bucket names and origin URIs, so we don't need to add another check. Moreover, if a bucket already exists, we want to show an error while importing, so that users know that bucket is there.

I agree that "if a bucket already exists, we want to show an error while importing", but currently the resulted warning and hint is misleading in this context, especially:
use "scoop bucket rm main" to remove it
"'$($Item.Name)' is already in your local bucket. It won't be added again." is more specific and user-friendly in the context of using a import function.

libexec/scoop-import.ps1 Outdated Show resolved Hide resolved
@zStruCat
Copy link
Contributor Author

zStruCat commented Jul 6, 2022

I've changed accordingly.
图片

lib/buckets.ps1 Outdated Show resolved Hide resolved
Make warning clearer

Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
@rashil2000 rashil2000 merged commit b4e0ff1 into ScoopInstaller:develop Jul 7, 2022
slaughtering pushed a commit to slaughtering/scoop that referenced this pull request Jul 7, 2022
…ity check (ScoopInstaller#5034)

* Add nullity check and alread_in_local_bucket check

* update CHANGELOG

* Use foreach instead of ForEach-Object

* changelog

* update help

* refine the info and warning when adding an already-added bucket

* Update lib/buckets.ps1

Make warning clearer

Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>

Co-authored-by: Rashil Gandhi <rashil2000@gmail.com>
Co-authored-by: Rashil Gandhi <46838874+rashil2000@users.noreply.github.com>
@zStruCat zStruCat deleted the fix-import branch July 7, 2022 13:41
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.

None yet

3 participants