Bash Strict Mode: Part 1 of 3 #817
Conversation
lib/commands/command-reshim.bash
Outdated
| generate_shims_for_version "$plugin_name" "$full_version" | ||
| asdf_run_hook "post_asdf_reshim_$plugin_name" "$full_version" | ||
| generate_shims_for_version "$plugin_name" "$full_version_name" | ||
| asdf_run_hook "post_asdf_reshim_$plugin_name" "$full_version_name" |
There was a problem hiding this comment.
This was actually a bug! These functions were invoked in a way that was inconsistent with the code in the else block below.
| @@ -80,5 +80,5 @@ teardown() { | |||
|
|
|||
| # Should not contain duplicate colon | |||
| run grep '::' <(echo "$path_line") | |||
| [ "$duplicate_colon" == "" ] | |||
There was a problem hiding this comment.
Another bug. This variable was never set so this line wasn't actually testing anything.
| @@ -119,7 +119,7 @@ teardown() { | |||
| @test "parse_asdf_version_file should output path on project with spaces" { | |||
| PROJECT_DIR="$PROJECT_DIR/outer space" | |||
| mkdir -p "$PROJECT_DIR" | |||
| cd $outer | |||
There was a problem hiding this comment.
Another test that wasn't testing what it was suppose to because of an unset variable.
|
looking over this now :) |
|
@jthegedus build is failing right now. I created this PR on my OSX machine but it looks like things aren't working on Ubuntu. I'm going pull these changes down on my Ubuntu machine and resume working through the remaining issues. I probably won't get to this until late next week though. |
|
No problem. This is a good start to enabling strict mode. I like this process has revealed bugs, proves the effort is valid 😄 |
| # Yes, the syntax for the expansion is ugly and confusing, but it is what we | ||
| # need to do to make it work with the nounset option. See | ||
| # https://stackoverflow.com/q/7577052/ for the details | ||
| "$env_cmd" ${env_args[@]+"${env_args[@]}"} |
There was a problem hiding this comment.
In an effort to capture minimum versions we support, I would like capture the min Bash version that supports this in the docs if we could
Yes, this is next on my agenda. I will rebase and try to fix the failing tests on Ubuntu tomorrow. |
Stratus3D
force-pushed
the
tb/no-unset-vars
branch
from
8e085bf
to
3f91ba9
Compare
Stratus3D
force-pushed
the
tb/no-unset-vars
branch
from
8a6a0c9
to
1c8648d
Compare
|
I just rebased this PR onto latest master. I'm hoping to finish this PR update and get it merged soon. |
|
Tag me when you wish to have a review. |
|
I am not sure if we can automate some of these fixes with Shellcheck/Shfmt, but perhaps these proposed changes can be enforced (and potentially auto-fixed) by expanding |
|
@Stratus3D should we close this? |
|
We can definitely add some stuff to Unfortunately, I can't think of any way to add the latter two to |
|
@hyperupcall I'm very happy with the level of code quality improvements you have brought to the project over the past year. I don't think this is worth tackling (and assumed Stratus would agree), so thought I would just ask before closing it. I will just close it though as I think we're all better off spending our time on other issues than this in the short term. We can always dig it back up should we want. |
|
Thanks @jthegedus. I don't have plans to tackle this anymore. I'm focusing my effort on the Rust rewrite. |
Summary
Instead of implementing Bash strict mode in one pull request (as I tried to do with #520 and failed) I am going to do it in three parts:
This PR addresses part of #160