asdf icon indicating copy to clipboard operation
asdf copied to clipboard

Published 20 hours ago • verified publisher icon asdf-vm

Bash Strict Mode: Part 1 of 3

Open Stratus3D opened this issue 4 years ago • 6 comments

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:

  • Don't allow unset variables (this PR)
  • Exit on non-zero exit codes (including in pipes)
  • Set a stricter IFS

This PR addresses part of #160

Stratus3D avatar Oct 16 '20 15:10 Stratus3D

looking over this now :)

jthegedus avatar Nov 09 '20 04:11 jthegedus

@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.

Stratus3D avatar Nov 09 '20 14:11 Stratus3D

No problem. This is a good start to enabling strict mode. I like this process has revealed bugs, proves the effort is valid :smile:

jthegedus avatar Nov 10 '20 09:11 jthegedus

Hey mate, can this be re-based? If I recall it was pretty close to done.

Yes, this is next on my agenda. I will rebase and try to fix the failing tests on Ubuntu tomorrow.

Stratus3D avatar May 27 '21 13:05 Stratus3D

I just rebased this PR onto latest master. I'm hoping to finish this PR update and get it merged soon.

Stratus3D avatar May 17 '22 13:05 Stratus3D

Tag me when you wish to have a review.

jthegedus avatar May 18 '22 00:05 jthegedus

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 scripts/stylecheck.py?

jthegedus avatar Apr 05 '23 14:04 jthegedus

@Stratus3D should we close this?

jthegedus avatar Jun 18 '23 07:06 jthegedus

We can definitely add some stuff to checkstyle.py to prepare things for set -u, and I feel reluctant to say this to a 3-year old PR adding this stuff, but in my opinion, errunset is more trouble than it's worth, especially with ShellCheck.

Unfortunately, I can't think of any way to add the latter two to checkstyle.py, but maybe there is a way.

hyperupcall avatar Jun 19 '23 05:06 hyperupcall

@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.

jthegedus avatar Jun 22 '23 16:06 jthegedus

Thanks @jthegedus. I don't have plans to tackle this anymore. I'm focusing my effort on the Rust rewrite.

Stratus3D avatar Jul 12 '23 20:07 Stratus3D