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.

Sign up for GitHub

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

Jump to bottom

[shell] warn when removing non-existent package #230

Merged
merged 1 commit into from
Oct 13, 2022
Merged

[shell] warn when removing non-existent package #230

merged 1 commit into from
Oct 13, 2022

Conversation

savil
Copy link
Collaborator

@savil savil commented Oct 12, 2022 •
edited
Loading

Summary

Previously, devbox would indicate that an uninstall happened even if a package wasn't part of the devbox.json packages. This isn't great, especially if a user made an inadvertent typo.

This PR prints a warning for such a scenario.

It also changes logic to print the confirmation messages outside devbox shell.

How was it tested?

> cd testdata/rust/rust-stable
> devbox shell
(devbox)> devbox add go_1_18 nodejs

# package doesn't exist
(devbox)> devbox rm php
Warning: the following packages were not found in your devbox.json: php
Uninstalling nix packages. This may take a while...done.
No packages removed.

# subset of packages exist 
(devbox) > devbox rm php rummy nodejs
Warning: the following packages were not found in your devbox.json: php, rummy
Uninstalling nix packages. This may take a while...done.
nodejs is now removed. Run `hash -r` to ensure your shell is updated.

# package exists
(devbox)> devbox rm go_1_18
Uninstalling nix packages. This may take a while...done.
go_1_18 is now removed. Run `hash -r` to ensure your shell is updated.

@savil Graphite App
Copy link
Collaborator Author

savil commented Oct 12, 2022

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested review from gcurtis and LucilleH October 12, 2022 22:53
@savil savil marked this pull request as ready for review October 12, 2022 22:54
@LucilleH LucilleH mentioned this pull request Oct 13, 2022
Merged
LucilleH
LucilleH approved these changes Oct 13, 2022
View reviewed changes
Copy link
Collaborator

@LucilleH LucilleH left a comment

Choose a reason for hiding this comment

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

👍

@savil
Copy link
Collaborator Author

savil commented Oct 13, 2022

@gcurtis @LucilleH Should the warning be to stdout or stderr?

@savil savil merged commit 618c0a8 into main Oct 13, 2022
@savil savil deleted the savil/rm-missing-pkg branch October 13, 2022 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LucilleH LucilleH LucilleH approved these changes

@gcurtis gcurtis Awaiting requested review from gcurtis

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@savil @LucilleH