Make bashly-generated code set -u-safe

[wolfgang42]

set -u ("no unset variables") is a useful shell option that catches a lot of bugs caused by things like misspelled variables. This PR updates all of the Bashly templates to use set -u-safe idioms so it can be set globally for bashly-managed scripts.

This is currently a DRAFT: there are some broken things (see PR comments) but filing it here for discussion.

[wolfgang42]

DANGER DANGER: This will obviously break a lot of scripts if merged in. I stuck this here for testing but I'm not sure what the best way to handle this is. Maybe just adding this to every example's initialize.sh?

[DannyBen]

We can generate this line conditionally, for example, if the environment variable BASHLY_STRICT is set. And then ENV['BASHLY_STRICT'] = '1' can be added to the spec helper.

This has the positive side effect of letting users generate strict scripts as well by just setting this variable.

[wolfgang42]

Would it make sense to put this as a flag like bashly generate --strict rather than an env var?

[DannyBen]

Originally, I suggested an env var, since I did not intend this to be a user-facing feature. Only enabled when the tests execute. If users want to add something, they have the initialize.sh for this - no need for new flags I think.

[DannyBen]

I am not so sure about the necessity of this.
There are other, less strict ways to catch bugs.

If users want it in their script, they can add it to the initialization sequence, and for bashly itself, I am not sure it is required.

Have you seen a bug that this would have helped catch?

[wolfgang42]

If users want it in their script, they can add it to the initialization sequence

This is the trouble. If you do that the generated script will crash because it runs set -u in initialize() and then immediately crashes in bashly-generated code while trying to parse arguments.

I do agree that this shouldn't be forced on users, hence my comment on initialize.erb about having added it there for testing (so I could run the specs and see what was crashing and needed to be fixed). I do think that all of the bashly tests should be run with this mode enabled, so that they can catch these problems.

for bashly itself, I am not sure it is required

You mean to catch bugs in bashly? I agree, I don't think this is very important for checking bashly itself, but it is important to have so that user scripts can have this option set without having to turn it off before calling into bashly-generated code.

Have you seen a bug that this would have helped catch?

Yes, this PR was prompted by me writing a bug that accidentally deleted a file because I made a mistake when writing the argument name. I then went to add set -u to my initializer to keep that from happening again and discovered that I couldn't do that because bashly was generating code that didn't work properly with this option set.

On a side note, I find it interesting that bashly sets -e automatically, especially in light of your comments about -u. Out of curiosity, what was the reason for setting that particular option?

[DannyBen]

Ok, I see your point now. You wish to make the generated scripts "set -u compliant", and not add "set -u" as a default. With this I can agree. Just need to have a PR clean of anything that is debatable.

Out of curiosity, what was the reason for setting that (set -e) particular option?

Setting set -e makes bash behave like other programming languages. When there is an error, it stops. This should be a default behavior in my view. Do you feel that it should not be in the initialize function?

[DannyBen]

BTW - I was thinking that perhaps another way to tackle this problem it to add an optional before and after function placeholders. This will run before and after calling any user code. If this was available, you could have placed your set -u in the src/before.sh partial.

This will have this logical order:

1. initialize (user injectable)
2. other bashly code
3. before (user injectable)
4. user code
5. after (user injectable)

[DannyBen]

By the way, I am still OK with continuing with this PR - if it only contains changes that make bashly set -u compatible, and nothing more (except of course the adjustments to the specs as needed).

If you want to clean it up, and mark it ready for review, I will merge (or if my help or attention is needed, please point me where I am needed).

[wolfgang42]

Putting this PR on hold pending discusison on #114 (comment)

[DannyBen]

As I see it now (after the recent merged PRs), this PR can become valid under these assumptions:

1. It is merged with the current state of master
2. It is cleaned up to only include changes that are require to make bashly compatible with set -u on bash 4.
3. Scripts generated pass bash 3 tests in their mint condition

Then, users who wish to include code that breaks parsing in bash 3, must implement a custom header.

If this is not possible, or complex - I suggest closing this PR. In this case, users can add set -whatever in their own code (and not in initialize.sh), and if/when pre-user-command hook is implemented, it can go there.

[wolfgang42]

That sounds like a reasonable approach; I'll try to make some time soon to clean up this PR to fix bash 3 support.

Also, continuing this discussion:

Out of curiosity, what was the reason for setting that (set -e) particular option?

Setting set -e makes bash behave like other programming languages. When there is an error, it stops. This should be a default behavior in my view. Do you feel that it should not be in the initialize function?

I don't have a strong opinion either way, I just feel like this may be an inconsistency. set -u also makes bash behave more like other programming languages (most of which require declaring or at least setting a variable before using it), so I was wondering why Bashly would turn on one but not the other.

[DannyBen]

I just feel like this may be an inconsistency. set -u also makes bash behave more like other programming languages.

Well - bashly is just a tad opinionated.
I adapt the Ruby motto - make developers happy. Making something more strict, usually doesn't.

Also, don't forget that bash was designed to allow these unset vars - as can be seen in this very PR - natural ways of writing things in bash, fail with this enabled.

[wolfgang42]

Per your note on #121 (comment), LMK what you decide about this PR. Happy to make it Bash 3-compatible as we discussed.

[DannyBen]

LMK what you decide about this PR. Happy to make it Bash 3-compatible as we discussed.

Well - if you are still up for it, and the resulting PR does not complicate things too much, and my points in this comment are met (assuming you agree they are valid) - sure, why not.

Happy to get this merged as well.

As for the short discussion around DANGER DANGER - I would like to implement it myself, after you finish your touches. You just add the necessary set ... directives there, as strict as possible, so we ensure tests pass at this state, and I will arrange the "test switch" for it.

[DannyBen]

@wolfgang42 - you were nowhere to be found for some of our other PRs. Is this one still happening?

[DannyBen]

@wolfgang42 - I see you are no longer available to continue this, and since the master branch has diverged too much, I could not resolve its conflicts.

I am closing this in favor of #148, which I had to recreate manually based on your code.

Thanks for your help.