Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

${parameter-default} vs ${parameter:-default} #871

Closed
nemchik opened this issue Jun 3, 2022 · 8 comments
Closed

${parameter-default} vs ${parameter:-default} #871

nemchik opened this issue Jun 3, 2022 · 8 comments

Comments

@nemchik
Copy link

nemchik commented Jun 3, 2022

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.

@mvdan
Copy link
Owner

mvdan commented Jun 6, 2022

See #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?

@scop
Copy link
Contributor

scop commented Jun 6, 2022

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.

@mvdan
Copy link
Owner

mvdan commented Jun 7, 2022

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

@nemchik
Copy link
Author

nemchik commented Jun 7, 2022

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?

@scop
Copy link
Contributor

scop commented Jun 8, 2022

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.

@nemchik
Copy link
Author

nemchik commented Jun 10, 2022

I think I'm happy to close this, unless anyone else feels the need to discuss the topic further.

@mvdan
Copy link
Owner

mvdan commented Jun 10, 2022

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.

mvdan added a commit that referenced this issue Jun 10, 2022
Some people are confused by `latest` versus `v3`, so clarify that.
Also document that we publish releases and Alpine variants.

While here, remove the reference to the alternative Docker image,
as these days it's somewhat redundant (we took ideas from it)
and also currently out of date.

See #740, #805, #871.
mvdan added a commit that referenced this issue Jun 11, 2022
Some people are confused by `latest` versus `v3`, so clarify that.
Also document that we publish releases and Alpine variants.

While here, remove the reference to the alternative Docker image,
as these days it's somewhat redundant (we took ideas from it)
and also currently out of date.

See #740, #805, #871.
@mvdan
Copy link
Owner

mvdan commented Sep 21, 2022

Closing per the above.

@mvdan mvdan closed this as not planned Won't fix, can't repro, duplicate, stale Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants