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

adding bash_completion logic #41

Closed
wants to merge 4 commits into from
Closed

adding bash_completion logic #41

wants to merge 4 commits into from

Conversation

komish
Copy link
Contributor

@komish komish commented Jun 8, 2017

Provides bash_completion functionality should someone find themselves using this via the cli for any reason.

Functionality should remove a short-flag if the corresponding long-flag has been called. Also should work if an argument takes a value.

Demonstration:

$ kthresher -<tab><tab>
--config            --help              --show-autoremoval  -H                  -d                  -p
--dry-run           --keep              --verbose           -V                  -h                  -s
--headers           --purge             --version           -c                  -k                  -v
$ kthresher --verbose -<tab><tab>
--config            --help              --show-autoremoval  -V                  -h                  -s
--dry-run           --keep              --version           -c                  -k
--headers           --purge             -H                  -d                  -p
$ kthresher --verbose -c file -<tab><tab>
--dry-run           --keep              --version           -d                  -p
--headers           --purge             -H                  -h                  -s
--help              --show-autoremoval  -V                  -k
$ kthresher --verbose -c file --dry-run

Did not specify install location, leaving it up to user or maintainer preference on whether it should be installed automatically or not.

@tonyskapunk
Copy link
Contributor

@komish thanks for the PR, I'll review and provide feedback as soon as I can.

@tonyskapunk tonyskapunk self-assigned this Jun 12, 2017
Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

Sorry it took me forever to review this code, added some comments for it.

local IFS
local not_used=""
# options in pairs
local opts="--help,-h --config,-c --dry-run,-d --headers,-H --keep,-k --purge,-p --show-autoremoval,-s --verbose,-v --version,-V"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make it fit in 80c, e.g.

local opts
opts="--help,-h --config,-c --dry-run,-d --headers,-H --keep,-k --purge,-p "
opts+="--show-autoremoval,-s --verbose,-v --version,-V"

# 0 if string in $1 is in the array
# 1 if not
local e
for e in "${@:2}"; do [[ "$e" == "$1" ]] && return 0 ; done
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do the for loop semi-multi-line use if and use {} on all variable references, to be consistent all across the code, e.g.

for e in "${@:2}"; do
  if [[ "${e}" == "${1}" ]]; then
    return 0
  fi
done

OLDIFS=$IFS
# set IFS to split our pairs
IFS=","
set -- $i
Copy link
Contributor

Choose a reason for hiding this comment

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

{} on variable

IFS=","
set -- $i
# reset IFS to original value
IFS=$OLDIFS
Copy link
Contributor

Choose a reason for hiding this comment

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

{} on variable

set -- $i
# reset IFS to original value
IFS=$OLDIFS
result_one=$(containsElement "${1}" "${COMP_WORDS[@]}" ; echo $?)
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be rewriten as:

containsElement "${1}" "${COMP_WORDS[@]}"
result_one="${?}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Went with the alternate suggested path here.

result_one=$(containsElement "${1}" "${COMP_WORDS[@]}" ; echo $?)
result_two=$(containsElement "${2}" "${COMP_WORDS[@]}" ; echo $?)
# if neither exists in COMP_WORDS, both short and long opt are valid for use
if [[ "${result_one}" != 0 ]] && [[ "${result_two}" != 0 ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

OR written like this

if ! $( containsElement "${1}" "${COMP_WORDS[@]}" ) &&
   ! $( containsElement "${2}" "${COMP_WORDS[@]}" ); then

not_used="${not_used} ${2}"
fi
done
echo "$not_used"
Copy link
Contributor

Choose a reason for hiding this comment

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

{} on variable

local cur=${COMP_WORDS[COMP_CWORD]}
local prev=${COMP_WORDS[COMP_CWORD-1]}
#echo "Genopts $(genOpts) COMP_WORDS ${COMP_WORDS[@]}"
COMPREPLY=( $(compgen -W "$(genOpts)" -- $cur ) )
Copy link
Contributor

Choose a reason for hiding this comment

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

{} on variable

@komish
Copy link
Contributor Author

komish commented Jul 25, 2017

updated to include stylistic elements as requested.

Copy link
Contributor

@tonyskapunk tonyskapunk left a comment

Choose a reason for hiding this comment

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

Few minor things

# set IFS to split our pairs
IFS=","
set -- $i
set -- ${i}``
Copy link
Contributor

Choose a reason for hiding this comment

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

is that extra `` a typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, I'll sort this out and resubmit.

if ! $( containsElement "${1}" "${COMP_WORDS[@]}" ) && \
! $( containsElement "${2}" "${COMP_WORDS[@]}" ); then
not_used="${not_used} ${1}"
not_used="${not_used} ${2}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments I did was on the if statement, I see that has been changed as suggested, great!, minor think with && there is no need for \ at the end of the line when having &&, || or | so one less char there :) .

What I missed in the first review is the assignment of not_used, is there any reason why two assignments are after each other? instead of appending both elements at once? e.g.

not_used="${not_used} ${1} ${2}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the extraneous \ - I also tried moving the && to the next line and leaving the \ back in as I feel that is probably more logical to the brain but the former looked easier to read so ultimately went with that suggestion.

I don't see a visible difference in functionality between append the new values in one line vs in two, I'm made that adjustment as well. PR update incoming.

@tonyskapunk
Copy link
Contributor

Please squash your commits into a single one, thanks!!!! 💃

@tonyskapunk
Copy link
Contributor

Closing as #50 takes care of it, (same changes, jus squashed) thanks @komish !!

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

2 participants