Shellcheck #223
Shellcheck #223
Conversation
|
Hey, looks like this PR has a lot of changes, could you please rebase on current master? |
|
Rebase complete. I have still not looked at the As I originally stated, I don't have full confidence in the changes but I felt it was better to do some of the legwork to get the project passing with shellcheck. I love the idea of a universal version manager and wanted to contribute in some way. Feel free to use, modify, or throw away my work so that it aligns with the project's goals. |
|
@jonknapp Thanks so much for your work on adding shellcheck! I've just shellcheckedd all of commands/*.sh and the test suite is green, so just let do a few manual tests (plugin-test) and I guess this would be ready for merge. Thanks again for working on this! |
|
@asdf-vm/maintainers I've reviewed this PR, made sure the tests are passing and tested locally using this branch (manually installing plugins, shims worked and So it would be great if anyone could also give a quick look at the PR just to be sure I'm not missing anything. I'd like to merge this if everything is ok. |
|
Thank you very much and sorry for the delay. |
|
Things are all fixed up now. There was one conflict and the linter was getting caught up on text output for a help description (which I fixed by disabling the check for that instance). I merged in |
|
Thank you very much for this. Thanks! |
|
done |
| elif [ -f /proc/cpuinfo ]; then | ||
| echo $(grep -c processor /proc/cpuinfo) | ||
| grep -c processor /proc/cpuinfo |
There was a problem hiding this comment.
I think the echos may still be needed here if something is capturing the output from this function.
There was a problem hiding this comment.
I'm not super well versed in bash scripting, so please feel free to let me know if something I suggest is incorrect. That said, I'm not sure why the echo is needed if the output of the grep command is sent to stdout/err already. It was a recommendation from shellcheck which may or may not apply here.
For the rest of comments, feel free to run this with shellcheck yourself to see the error output. I put a lint.sh file in the root of the project that lints the project files.
| exit_code=$? | ||
| if [ $exit_code -ne 0 ]; then |
There was a problem hiding this comment.
Did shellcheck suggest this? I was thinking -ne was a more widely supported condition.
There was a problem hiding this comment.
Yea, I didn't make any changes that were not suggested by shellcheck. If they are syntactically correct I would tend to error on the side of shellcheck and there may be more info about why it was suggested on the rule definition doc page.
lib/utils.sh
Outdated
| else | ||
| echo $(asdf_dir)/installs/${plugin}/${install_type}-${version} | ||
| echo "$(asdf_dir)"/installs/"${plugin}"/"${install_type}"-"${version}" |
There was a problem hiding this comment.
In both of these cases double quotes around the entire path should suffice.
There was a problem hiding this comment.
Sounds good. There may be more instances of this as well. I'll poke around before committing the code.
lib/utils.sh
Outdated
| display_error "version $version is not installed for $plugin_name" | ||
| exit 1 | ||
| fi | ||
| } | ||
|
|
||
| get_plugin_path() { | ||
| echo $(asdf_dir)/plugins/$1 | ||
| echo "$(asdf_dir)"/plugins/"$1" |
There was a problem hiding this comment.
Again, it's easier if you wrap the whole path in a single set of double quotes.
|
Made the corrections for unnecessary string quoting and added comments to the other feedback items. Feel free to fork and make additional changes (I believe asdf maintainers can change my Github branch) or let me know what else should be touched up. |
|
Going to merge now. I'll try out locally later this week. |
Summary
This brings shellcheck support to the application by allowing linting of the project's source through the
./lint.shcommand. Linting is also added to Travis.Fixes: Hopefully, this will fix #202 (which may not be fixed) as well as similar issues
(Historical) Other Information
This is not ready in its current form as I've only updated issues found outside of the
libfolder for the project. I'm also not 100% confident in my Bash scripting skills or in my familiarity with the project source, so I wanted to start a PR early on the subject in case someone is interested in assisting (or picking it up from me). I've been relying on the test suite to verify if functionality is still working after correcting suggested linting errors.There will also be linting issues that I'm unsure how to solve and may add comments to ignore them in shellcheck. Feel free to make comments at any point if a different approach is desired to my changes or my ignored checks.
Solves #157