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

syntax: configurable formatting of opening braces #229

Closed
abhijithda opened this issue Apr 19, 2018 · 32 comments
Closed

syntax: configurable formatting of opening braces #229

abhijithda opened this issue Apr 19, 2018 · 32 comments
Milestone

Comments

@abhijithda
Copy link

abhijithda commented Apr 19, 2018

Thanks for the great tool. I have started using this tool with the VS Code.
I would like to provide one feedback w.r.t formatting functions (which would at least help couple of us).

The opening brace of the function definition should be in next line.

Currently it's as below...

function fun() {
}

Suggested way...

function fun()
{
}

Refer examples here: https://www.shellscript.sh/functions.html

One advantage of following the suggested format is, in the terminal editors like (vim), you can jump between functions by pressing [ and ] character twice. This is very important feature for specially scripts - as once you are done developing the script on your desktop, you would copy/run it on a system where there may not be graphical interfaces or IDEs.

@mvdan mvdan changed the title Formatting shell script functions syntax: configurable formatting of opening braces Apr 19, 2018
@mvdan
Copy link
Owner

mvdan commented Apr 22, 2018

I had written a comment to this, but apparently the shitty airport wifi decided to drop it :)

The reason this was not configurable from the start is to avoid having dozens of knobs for every possible style out there. For example, see the monstrosity that is indent: https://linux.die.net/man/1/indent

However, we have -bn for placing binary operators at the start of lines, so I can see that there is some precedent and symmetry in adding this option too.

I propose an option that will make all opening reserved words be placed on new lines. That is:

if foo
then
    bar
fi

foo()
{
    bar
}

This would include {, then, and do. I cannot think of others off the top of my head, but if you can think of any more, please do let me know.

@harleypig
Copy link

man bash, search for RESERVED and you get this list:

! case coproc do done elif else esac fi for function if in select then until while { } time [[ ]]

I can see a couple of issues: if ! condition || [[ someother condition ]]; then ... where and how many of these reserved words get a line break?

Also, I would think you'd want the closing reserved words to line up with the opening reserved words, correct?

Finally, how would you resolve [[ condition ]] && { echo 'msg'; exit1; }?

@mvdan
Copy link
Owner

mvdan commented Apr 23, 2018

Let me refine my definition above; all the command list opening reserved words. I see little value in doing it for reserved words that open to other stuff, such as in or [[.

There's also the question of what should happen with programs like:

foo() { bar; }

if foo; then bar; fi

At the moment, the formatter leaves those alone. The reasoning is that it is okay to have single-liners as long as they don't involve multiple sequential commands.

If this new option is enabled, should we forbid one-liners to enforce words like { to always be at the start of a line?

@brlin-tw
Copy link

brlin-tw commented May 5, 2018

I would really rather to eliminate the extra space between the closing parenthesis and the opening braces, id. est.:

function fun(){

}

just personal preference with no particular in practice reasons.

@mvdan
Copy link
Owner

mvdan commented May 5, 2018

That's a separate point, and seems only related to functions. But to answer your question, I think it's a bad idea. Braces are reserved words, so they should always be surrounded by whitespace for consistency (and also readability).

@abhijithda
Copy link
Author

abhijithda commented May 11, 2018

If it's possible only the { for the function opening needs to be updated. Rest all looks good to me as-is. I.e., They don't have to be moved to new line (as otherwise it will increase the number of lines of code). But I guess, that's fine as well, as my usecase is mainly to be able to quickly traverse between functions in a vim like editor on terminals where we can't have any graphical editors...

@mvdan
Copy link
Owner

mvdan commented Jun 1, 2018

The more I think about this, the less clear it is to me.

@harleypig just to clarify - are you in favor of this change, or against? Re-reading your comment, you raise valid points, and you make me think that a general "align all opening reserved words" would be messy.

Doing this only for function declarations is a simpler option, but then we are left with inconsistent formatting:

foo()
{
    bar
}

if foo; then
    bar
fi

foo && {
   bar
}

@mvdan
Copy link
Owner

mvdan commented Jun 1, 2018

Is there any style/formatting guide that enforces placing { or any other characters on separate lines? If so, exactly what are the rules it enforces?

@alok
Copy link

alok commented Jun 8, 2018

I've not seen a style guide outside the linux kernel that does that. I'd take a page from go's book and leave this nonconfigurable.

@mvdan
Copy link
Owner

mvdan commented Jun 8, 2018

@alok we've diverged too much from the "go mantra" of not having a configurable format, I'm afraid. If you look at the very first versions of shfmt, you'll notice that it had no formatting options. I ended up giving up on that, since the ship has sailed and there are too many conflicting style guides out in the wild.

That being said, I of course don't add every formatter knob that gets proposed. I'm still on the fence about this one, leaning on not adding it because it's unclear exactly what we want to add (see my comment above).

@dcsobral
Copy link

The braces thing is a VERY old discussion -- Berkeley vs AT&T Unix, referred to Allman vs K&R. Look up Indentation style on wikipedia.

It's a matter of taste. Allman is symmetrical in that open and closing braces have the same alignment. K&R is more economical, and aligns the start and end of the block, the asymmetry being inherent in what starts and ends a block.

Which is to say it can be reasonably be argued both ways, which means the discussion cannot be "won", which means it should be configurable. There's a conundrum in formatting tools: the best people to work on them are the ones not too opinionated on style, but the people more likely to work on it are the ones very opinionated about it.

I would avoid having all the block opening being behind a single flag. For one thing, test statements blocks begin with two statements, whereas functions have only one statement (but the very distinguishable braces being part of the opening one).

And if you went with a general rule, then these two ought to go together:

if COND; then
case WORD in PATTERN)

But I never saw a style guideline like that.

If you want to help people maintain a uniform style in their codebase, you need many individual "knobs" of configuration. Otherwise, this is going to be a great tool for people who happen to agree, or at least accept in lieu of another tool, the non-configurable choices made by it.

@dcsobral
Copy link

@mvdan I'd suggest the Go mantra of not having a configurable format does not apply to shell. It's something you can opt to do when you first design a language, but to do so when there's already a host of different styles will not accomplish anything but limit the tool usage.

@adriangrigore
Copy link

I'm afraid. If you look at the very first versions of shfmt, you'll notice that it had no formatting options

How did you choose the defaults?

@mvdan
Copy link
Owner

mvdan commented Jun 13, 2018

The braces thing is a VERY old discussion

Yes, I'm aware of the many styles of C-like languages. Note that my current concerns are:

  • Should we do this for only function declarations, only for braces, or for all tokens (including then, do, etc)?
  • Are there any existing and popular style guides that we can base this feature on?

I'd suggest the Go mantra of not having a configurable format does not apply to shell.

That is precisely what I was saying in my last comment. However, as the comment says, that doesn't mean that I'll add every knob and feature under the sun. The more knobs are added, the more complex the tool becomes and the more difficult it is to maintain. Hence why I'm trying to find out exactly what we want, and if it's worth adding it.

How did you choose the defaults?

A mix of looking at existing style guides (man bash, POSIX spec, Google's style guide), simplicity, and consistency. For example, I chose tabs as a default because the language works better with them via <<-.

@adriangrigore
Copy link

Should we do this for only function declarations, only for braces, or for all tokens (including then, do, etc)?

I would do it POSIX even though I'm a fan of K&R:

# repeat a command 100 times
x=100
while [ $x -gt 0 ]
do
    command    x=$(($x-1))
done
while
    # a couple of <newline>s


    # a list
    date && who || ls; cat file
    # a couple of <newline>s


    # another list
    wc file > output & true


do
    # 2 lists
    ls
    cat file
done
for i in *
do
    if test -d "$i"
    then break
    fi
done
foo() {
    for j in 1 2; do
        echo 'break 2' >/tmp/do_break
        echo "  sourcing /tmp/do_break ($j)..."
        # the behavior of the break from running the following command
        # results in unspecified behavior:
        . /tmp/do_break


        do_continue() { continue 2; }
        echo "  running do_continue ($j)..."
        # the behavior of the continue in the following function call
        # results in unspecified behavior (if execution reaches this
        # point):
        do_continue


        trap 'continue 2' USR1
        echo "  sending SIGUSR1 to self ($j)..."
        # the behavior of the continue in the trap invoked from the
        # following signal results in unspecified behavior (if
        # execution reaches this point):
        kill -s USR1 $$
        sleep 1
    done
}
for i in 1 2; do
    echo "running foo ($i)..."
    foo
done
for i in *
do
    if test -d "$i"
    then continue
    fi
    printf '"%s" is not a directory.\n' "$i"
done

In complete honesty, I would have done it POSIX by default , and build up on that (if it were the case or not).

I think:

foo()
{
    bar
}

is a pretty rare style. On a quick glance on https://github.com/dspinellis/unix-history-repo I didn't encounter it.

I'd suggest the Go mantra of not having a configurable format does not apply to shell.

It may me possible but it depends on @mvdan willingness to refuse feature requests.

I think it's better to have a standard on which most people agree (or make a dictatorial decision 😛 ) rather than have configuration options.

@mvdan
Copy link
Owner

mvdan commented Dec 2, 2018

Thanks everyone for the input - sounds like there isn't an agreement that we need this option. More importantly, noone has brought up a single shell style guide that encourages this kind of formatting. Or at least, a big shell codebase that follows this format.

We might add this option at some point if there are enough good reasons to add it. As it stands now, the only objective point in its favor is how some editors like Vim can make use of { at the beginning of lines.

Every flag added to the printer increases its complexity exponentially, as there are more and more combinations of flags that must be tested and understood well. This is a cost that is apparent to maintainers the most, and I don't think I'll get paid to maintain this software in the long run, so when in doubt I'll keep the number of knobs to a minimum :)

Closing for now. Happy to reopen if anyone brings more objective evidence.

@mvdan mvdan closed this as completed Dec 2, 2018
@oliv3r
Copy link
Contributor

oliv3r commented Dec 12, 2019

Thank you for a great tool like this. It is is much appreciated, however I do agree with some posters here, and I think this was not brought up more, as people tend to not care too much for shell scripts, and less about (auto) formatting. Aren't most shell scripts commonly function-less? (Checking out my debian's /etc/init.d I see even a mix of both styles in the same file ;)

I fully understand your desire not wanting to add hundreds of nobs, it makes the code hard to maintain.
On the other hand however, unlike go and python, there is hard style guide, heck there's so many variations around, as you rightfully say, it is not fair to enforce any standard.

However with that said, I'm not sure I quite follow your reasoning to not do it. It is clear you don't want to push for the 'mvdan' standard, which is good of course.

My reasoning for wanting this option is of course selfish ;). As I write a lot of (kernel style) C code, I've adapted one of the shell-styles matching this closely. I like the argument, functions are different and so lets mark them clearly as such. Also, I don't feel like 'fixing' dozens shell scripts in random places to match this style. Obviously, it is selfish and personal. This is how I like my code. Now if the request would be super strange to support some extremely odd style, I'd get it, lets not do it. But asking 'having function opening braces on the next line as an option' is not a horrible request to have.

Worst case scenario, would you be open to a pull-request implementing this feature?

@mvdan
Copy link
Owner

mvdan commented Dec 27, 2019

Thanks @oliv3r for your comment. @MorganAntonsson also raised a patch in #463, which is unfortunate as it seems he missed this thread entirely.

I'm going to reopen this issue for a few months to see if we can get some consensus. I want to clarify that I'll consider one printer option at most.

Question number one to resolve: is there a well defined or popular formatting style here that we should look at? For example, if it's just K&R, then the feature would only affect function declarations, and nothing else.

Question number two: what "opening tokens" would this feature affect? Only the { in function declarations? All opening { tokens? Or all opening tokens in the entire language, including then and do?

Please read the rest of the thread before posting a reply, and please keep your replies concise. I realise this thread is long already, which is why I want to keep it from going in circles. If anyone has a specific idea that they'd like to lay out properly, I encourage you to file a new issue and link it from here.

@mvdan mvdan reopened this Dec 27, 2019
@abhijithda
Copy link
Author

Question number one to resolve: is there a well defined or popular formatting style here that we should look at? For example, if it's just K&R, then the feature would only affect function declarations, and nothing else.

K&R style is my preferred option. The Linux kernel style follows similar formatting. Basically requesting this format to make jumping between functions easier on servers which don't have GUI interfaces, and options are most likely vim.

Question number two: what "opening tokens" would this feature affect? Only the { in function declarations? All opening { tokens? Or all opening tokens in the entire language, including then and do?

Yes, only { in function declarations.

@oliv3r
Copy link
Contributor

oliv3r commented Jan 8, 2020

Thanks @oliv3r for your comment. @MorganAntonsson also raised a patch in #463, which is unfortunate as it seems he missed this thread entirely.

I'm going to reopen this issue for a few months to see if we can get some consensus. I want to clarify that I'll consider one printer option at most.

Thank you for taking the time to read my rant and re-opening the issue.

Question number one to resolve: is there a well defined or popular formatting style here that we should look at? For example, if it's just K&R, then the feature would only affect function declarations, and nothing else.
As far as I know, there's K&R, which affects only function declarations, and the more 'bash-y' style I suppose?

Ironically, it took me a while, (many years ago) to get used to K&R style as we learned, for C, to put the opening brace of everything on the same line, including of functions. This was before I read K&R. I blame poor teachers ;)

Question number two: what "opening tokens" would this feature affect? Only the { in function declarations? All opening { tokens? Or all opening tokens in the entire language, including then and do?
Only for functions. I have not seen any other 'conflicting' styles, other then the already coverd 'then/do' on the same/next line.

So I think I am in complete agreement with @abhijithda here :)

@mvdan
Copy link
Owner

mvdan commented Jan 18, 2020

Ok, so just to reiterate, we're agreeing on adding one option to place opening braces on a separate line for function declarations. That is:

foo()
{
    body
}

function foo()
{
    body
}

if foo; then # not affected
    bar
fi

I see two options to implement this:

  1. Obey what the user does for opening braces in function declarations. This has the big upside of not requiring a flag, but the downside of not enforcing consistency.
  2. Add a flag. With what name? Unfortunately, "brace next line" conflicts a bit with "binary next line", which is -bn. I don't like -bnl, because it's still confusing. How about -fn, for "function next line"?

If anyone objects to this feature entirely, or has a strong opinion towards either of the two options above, please speak up before the end of the month.

@harleypig
Copy link

Somewhere along the way I picked up 'cuddled' for having the brace on the same line as the function, so '--no-cuddle' for breaking "foo() {" into separate lines.

@mvdan mvdan added this to the 3.1.0 milestone Feb 17, 2020
@mvdan mvdan closed this as completed in c3b7a40 Feb 17, 2020
@mvdan
Copy link
Owner

mvdan commented Feb 17, 2020

@MorganAntonsson's work is now merged! Please test it now in master, as 3.1.0 will probably release at some point in March. Once released, it's going to be impossible for us to make any breaking changes to the feature.

@arnaudlacour
Copy link

The more I think about this, the less clear it is to me.

@harleypig just to clarify - are you in favor of this change, or against? Re-reading your comment, you raise valid points, and you make me think that a general "align all opening reserved words" would be messy.

Doing this only for function declarations is a simpler option, but then we are left with inconsistent formatting:

foo()
{
    bar
}

if foo; then
    bar
fi

foo && {
   bar
}

I agree with @mvdan about the inconsistency this introduced.
Inconsistency is the enemy of legibility.

There are a lot of opinions about style being expressed here and at the end of the day, you can't argue simple preference.
But consistency is not a debatable state: it is or it is not.

@mvdan
Copy link
Owner

mvdan commented Jun 16, 2021

I'm open to consider turning -fn into a flag that also puts tokens like then and { in new lines. We'd name it something else, yet to determine. That could happen in v4 while removing -fn, or in v3 while simply deprecating -fn until v4.

To the people who participated in this thread in the past: please give a thumbs up or down here to show support in favor or against of doing this. If the reactions are mixed or generally in favor, I'll open a new issue and we can continue there. If the reactions are overwhelmingly against it, then I probably won't :)

@harleypig
Copy link

@harleypig just to clarify - are you in favor of this change, or against? Re-reading your comment, you raise valid points, and you make me think that a general "align all opening reserved words" would be messy.
Doing this only for function declarations is a simpler option, but then we are left with inconsistent formatting:

foo && {
   bar
}

Why would that last one have only 3 spaces for indentation? I'm not seeing where that change comes from.

I would prefer more options, but I'm not the one maintaining the code. :) I gave mvdan's comment a thumbs-up.

@mvdan
Copy link
Owner

mvdan commented Jun 16, 2021

Why would that last one have only 3 spaces for indentation? I'm not seeing where that change comes from.

Writing tabs in a web page textbox is pretty hard, so most people (myself included) just use spaces. I don't think you should pay attention to indentation in this thread, as it's mostly irrelevant.

@arnaudlacour
Copy link

arnaudlacour commented Jun 16, 2021

I'm open to consider turning -fn into a flag that also puts tokens like then and { in new lines.

I really appreciate the open-mindedness here and willingness to reconsider an issue now closed over a year ago.
It should be noted I'm not overly opinionated about one way or the other.
I am however about not mixing the two.

This having been said, I have the overall notion that the default recommendations go in a better direction.

@mvdan
Copy link
Owner

mvdan commented Jun 16, 2021

I am however about not mixing the two.

To be clear, you mean you don't think we should join "put opening tokens after a newline" under a new flag, including the current -fn? Assuming that you want to put tokens like then after a newline, I'd imagine you would be in support.

This having been said, I have the overall notion that the default recommendations go in a better direction.

I generally think that the formatting defaults are what most people should use, but opinions vary :)

@arnaudlacour
Copy link

arnaudlacour commented Jun 16, 2021

To be clear, you mean you don't think we should join "put opening tokens after a newline" under a new flag, including the current -fn? Assuming that you want to put tokens like then after a newline, I'd imagine you would be in support.

correct

@arnaudlacour
Copy link

I generally think that the formatting defaults are what most people should use, but opinions vary :)

after discussing a few things both with you and with my team, I've come to be of that opinion as well.

@mvdan
Copy link
Owner

mvdan commented Aug 16, 2021

If the reactions are mixed or generally in favor, I'll open a new issue and we can continue there.

Three thumbs up and no thumbs down, so I've filed it: #721

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants