Tab completion support for additional archives for 'borg delete' #5655
Conversation
Bash and Fish tab completions now too support more than just one archive provided for 'borg delete'.
|
can anybody familiar with completions review this please? |
|
@oxiedi maybe? |
|
I know almost nothing about Bash's completion system and Fish, thus I can only review code in bash. |
| # If there is a space before the "::" it means that no repository name was typed, | ||
| # so probably $BORG_REPO was set and we can still list the archives. | ||
| local repository_name=`expr match "${COMP_LINE}" "\(.*\)::"` | ||
| repository_name=${repository_name##* } |
There was a problem hiding this comment.
No need to call an external command and hide its return code in order to do some matching. Can be replaced with local repository_name="${COMP_LINE%%::*}". It would be better to match against the second positional argument, but I don't how to do it using bash-completion.
There was a problem hiding this comment.
Nice one, thanks for that!
scripts/shell_completions/bash/borg
Outdated
| local typed_word="" | ||
| local please_list_the_archives=false |
There was a problem hiding this comment.
It's common to use arithmetic expressions as booleans:
local -i list_archives=0
It's unusual to use articles in variable names. Also, the please part doens't seem to serve any purpose. Plain list_archives maybe?
There was a problem hiding this comment.
Well, I just tried to be funny 😄 On the other hand I did come up with list_archives first, but it seemed not to express the boolean nature of the variable and it was too similar to archive_list which is a list indeed.
scripts/shell_completions/bash/borg
Outdated
| # so probably $BORG_REPO is set. | ||
| repository_name="" | ||
| fi | ||
| if [[ $please_list_the_archives = true ]] ; then |
There was a problem hiding this comment.
If you are going to use an arithmetic expression as boolean, than it can be done like thie:
if (( $list_archives )); then
If not, then one equal sign is inconsistent with the previous conditions (should be ==).
There was a problem hiding this comment.
My goodness, that's a bug for sure, thanks for catching it. I'll fix this and also think about what you said of booleans.
There was a problem hiding this comment.
It's not a bug, just a minor inconsistency.
There was a problem hiding this comment.
Oh I thought it would behave like in C as it becomes an assignment instead of comparison.
|
I've manually tested it and with a few assumptions (repository should have the |
|
just tell me when this / the other PR is ready to merge. |
|
It is now ready, maybe @oxiedi can check it again? |
@ThomasWaldmann I think it's ready. |
|
Thanks! |
Bash and Fish tab completions now too support more than just one archive provided for 'borg delete'.