sh
sh copied to clipboard
mvdan
${parameter-default} vs ${parameter:-default}
I noticed that shfmt 3.5.1 (maybe also 3.5.0, I haven't tested yet) wants ${parameter-default} which has different behavior from ${parameter:-default}
Ref: https://tldp.org/LDP/abs/html/parameter-substitution.html
I believe this change should be reverted. Both methods have different purposes and should be used accordingly.
See https://github.com/mvdan/sh/pull/849; cc @scop.
I personally lean towards reverting, because the simplification is very subtle, but I'm not sure that it's actually an incorrect simplification given what the PR author explained.
Do you have an example of a script that actually breaks from this change?
Please note that we do not simplify ${parameter:-default} to ${parameter-default} unless default is literal null, i.e. the empty string. In other words, only ${parameter:-} gets simplified to ${parameter-}. For other cases doing it would be wrong indeed.
Aside, the change that adds this simplification is not, as far as I can tell, in 3.5.0 nor 3.5.1 yet. It's not in the v3.5 branch from where I gather v3.5.1 was released, and it's in master only after v3.5.0 and v3.6.0-0.dev.
@scop is right: the change is in master, but not in v3.5.1. It's not a bug fix, so it wasn't cherry-picked for a bugfix release.
I see now. That all makes sense. I apparently was using the latest tag on docker, which appeared to be 3.5.1 at the time, but I guess it's actually commits from the master branch?
Anyway given the explanation you provided above, I don't feel like the syntax without : is wrong the way it's applied here, but I also don't think it would be wrong to keep the :. Is there a flag or some kind of configuration for this to be optional?
In my view that flag is -s (or choosing not to use it) -- it already simplifies a bunch of other things that aren't wrong either. Some might be more opinionated than others, but then again so is the whole concept of code formatting.
I think I'm happy to close this, unless anyone else feels the need to discuss the topic further.
I'll leave this open for another week to see if anyone else has a new opinion or data to change our behavior. For now, I think we can keep the feature in master.