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

bash: fix shellcheck errors #889

Merged
merged 1 commit into from
Jul 11, 2019
Merged

Conversation

rsteube
Copy link
Contributor

@rsteube rsteube commented Jun 19, 2019

Fixed shellcheck errors according to documentation:

https://github.com/koalaman/shellcheck/wiki/SC2207
https://github.com/koalaman/shellcheck/wiki/SC2164

untested - tests are ok but someone should verify this is correct

@@ -72,7 +72,7 @@ __%[1]s_handle_reply()
else
allflags=("${flags[*]} ${two_word_flags[*]}")
fi
COMPREPLY=( $(compgen -W "${allflags[*]}" -- "$cur") )
mapfile -t COMPREPLY < <(compgen -W "${allflags[*]}" -- "$cur")
Copy link
Contributor

Choose a reason for hiding this comment

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

mapfile is not present in all bash versions. Namely, the out-of-the-box bash for MacOS doesn't have it (it's at a 3.0 version, it looks like mapfile was introduced in 4.0).

read -a COMPREPLY looks like it'll do the trick

Copy link
Contributor Author

@rsteube rsteube Jun 20, 2019

Choose a reason for hiding this comment

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

I think i need a project where i can test this. Disabling escape characters using -r (shellcheck warning) and the IFS seems to be required as well for it to work. So it's probably one of:

# For bash 3.x+
array=()
while IFS='' read -r line; do array+=("$line"); done < <(mycommand)

or

# For bash
IFS=" " read -r -a array <<< "$(mycommand)"

Some tests with read -a in a bash shell didn't give me the right result (or i simply used it wrong).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it seems read -a really doesn't work as expected. I've tested this on multiple bash versions, and it seems to be the most consistent:

while read -s -d " " c ; do
  COMPREPLY+=("$c")
done < <(compgen -W "${allflags[*]}" -- "$cur")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it need to be cleared first?: COMPREPLY=()

Copy link
Contributor

Choose a reason for hiding this comment

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

Bash will automatically create the array variable if you start appending to it:

unset a
a+=("foo")
echo "${a[@]}"
foo

But it wouldn't hurt to make sure it's empty anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was just worried that it is defined before already, but should be fine then.
Just compiled https://github.com/gohugoio/hugo with it but seems there is still sth. wrong with the completion (https://gist.github.com/rsteube/7647a9a3456cdb102f0b88ba004d74f8).
Currently looking into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while IFS='' read -r c; do works in my shell (5.0.7).
https://gist.github.com/rsteube/6bb3336adf902193997bba048094b870

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bash_completions.go Outdated Show resolved Hide resolved
@umarcor
Copy link
Contributor

umarcor commented Jul 6, 2019

@spf13, @eparis, can we please have this merged so that Travis CI jobs are fixed for every other PR based on master?

@jharshman
Copy link
Collaborator

@rsteube, while I review this, can you squash your commits?

@rsteube
Copy link
Contributor Author

rsteube commented Jul 10, 2019

@jharshman done

@jharshman
Copy link
Collaborator

jharshman commented Jul 10, 2019

@rsteube thanks, I'm just going to do a bit of testing of the bash completions before I merge this in. I would encourage you to do the same if you haven't yet. I'm not seeing any breaking changes here but just to be sure.

@rsteube
Copy link
Contributor Author

rsteube commented Jul 10, 2019

Recompiled hugo with the squashed version and no changes to https://gist.github.com/rsteube/6bb3336adf902193997bba048094b870 which seemed to be fine in bash 5.0.7 and 3.2.57.

@jharshman
Copy link
Collaborator

Validated by recompiling kubectl.

@jharshman jharshman merged commit 21f39ca into spf13:master Jul 11, 2019
@rsteube rsteube deleted the fix-shellcheck branch July 11, 2019 19:51
scottcarol pushed a commit to scottcarol/cobra that referenced this pull request Jul 11, 2019
bash: fix shellcheck errors (spf13#889)
scottcarol pushed a commit to scottcarol/cobra that referenced this pull request Jul 11, 2019
bash: fix shellcheck errors (spf13#889)
marckhouzam added a commit to marckhouzam/cobra that referenced this pull request Dec 12, 2019
PR spf13#889 introduced a regression where the global variable $c is no
longer set when *custom_func is called.  This is because $c is re-used
by mistake in the read loop.

This PR simply changes the name of the variable used in the loop.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
jharshman pushed a commit that referenced this pull request Dec 26, 2019
PR #889 introduced a regression where the global variable $c is no
longer set when *custom_func is called.  This is because $c is re-used
by mistake in the read loop.

This PR simply changes the name of the variable used in the loop.

Signed-off-by: Marc Khouzam <marc.khouzam@gmail.com>
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.

4 participants