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

Add unalias --all #2615

Merged
merged 13 commits into from
Jun 24, 2022
Merged

Add unalias --all #2615

merged 13 commits into from
Jun 24, 2022

Conversation

luis4a0
Copy link
Contributor

@luis4a0 luis4a0 commented Jun 14, 2022

This PR adds the argument --all to the unalias command. It also makes unalias capable to remove various alias at once.

Addresses #2298.

@codecov
Copy link

codecov bot commented Jun 14, 2022

Codecov Report

Merging #2615 (e38e271) into main (222a4c0) will increase coverage by 0.32%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2615      +/-   ##
==========================================
+ Coverage   86.14%   86.46%   +0.32%     
==========================================
  Files         217      217              
  Lines       10794    10810      +16     
==========================================
+ Hits         9298     9347      +49     
+ Misses       1496     1463      -33     
Impacted Files Coverage Δ
src/client/cli/cmd/unalias.h 100.00% <ø> (ø)
include/multipass/cli/alias_dict.h 100.00% <100.00%> (ø)
src/client/cli/cmd/unalias.cpp 100.00% <100.00%> (ø)
src/client/common/alias_dict.cpp 100.00% <100.00%> (ø)
src/platform/backends/lxd/lxd_virtual_machine.cpp 90.05% <0.00%> (-0.27%) ⬇️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 71.21% <0.00%> (-0.06%) ⬇️
...tform/backends/libvirt/libvirt_virtual_machine.cpp 63.34% <0.00%> (+0.11%) ⬆️
src/daemon/daemon.cpp 61.53% <0.00%> (+2.37%) ⬆️
src/platform/update/disabled_update_prompt.h 100.00% <0.00%> (+33.33%) ⬆️
src/daemon/daemon.h 100.00% <0.00%> (+100.00%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 222a4c0...e38e271. Read the comment docs.

@luis4a0 luis4a0 force-pushed the add-unalias-dashdash-all branch from 71096c7 to 99eaeed Compare June 22, 2022 11:37
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Works, well, thanks! Just some comments inline to address.

include/multipass/cli/alias_dict.h Outdated Show resolved Hide resolved
src/client/cli/cmd/unalias.cpp Outdated Show resolved Hide resolved
src/client/cli/cmd/unalias.cpp Outdated Show resolved Hide resolved
src/client/cli/cmd/unalias.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Thanks, almost there!

One thing I found is that the same alias is allowed to be entered multiple times and then the following error is shown:
Warning: 'No such file or directory' when removing alias script for mp-ls

IMO, duplicate aliases should just be ignored and treated as a single alias.

Also, and I don't know if it's really a big deal if we handle duplicate aliases like I propose above, but the unalias bash completion will show the same alias as an alias to choose even if it's already selected in the command line.

luis4a0 added 2 commits June 23, 2022 14:47
This way, we avoid repetitions in the list of alias to remove, thus
eliminating the repeated removal of alias scripts.
@luis4a0
Copy link
Contributor Author

luis4a0 commented Jun 23, 2022

Thanks! As for the repeated elements in the arguments, now I store them in an unordered_set instead of in a vector. This way, each alias to remove is stored only once.

I'll see what can be done with the completions (I implemented something similar for network interfaces so far).

@luis4a0
Copy link
Contributor Author

luis4a0 commented Jun 23, 2022

Done the change in bash completions. Now, each alias is suggested only once. This can be refactored in the future and be reused in commands on which we don't want to repeat (and to avoid repeating options like --all or --help.

Copy link
Contributor

@townsend2010 townsend2010 left a comment

Choose a reason for hiding this comment

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

Ok, great, thanks!

bors merge

bors bot added a commit that referenced this pull request Jun 23, 2022
2615: Add `unalias --all` r=townsend2010 a=luis4a0

This PR adds the argument `--all` to the `unalias` command. It also makes `unalias` capable to remove various alias at once.

Addresses #2298.

Co-authored-by: Luis Peñaranda <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 23, 2022

Build failed:

@townsend2010
Copy link
Contributor

bors retry

bors bot added a commit that referenced this pull request Jun 24, 2022
2615: Add `unalias --all` r=townsend2010 a=luis4a0

This PR adds the argument `--all` to the `unalias` command. It also makes `unalias` capable to remove various alias at once.

Addresses #2298.

Co-authored-by: Luis Peñaranda <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jun 24, 2022

Build failed:

@townsend2010
Copy link
Contributor

Ok, let's try one more time...

bors retry

@bors bors bot merged commit 70b5e25 into main Jun 24, 2022
@bors bors bot deleted the add-unalias-dashdash-all branch June 24, 2022 14: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