Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants