Repository: mvdan/sh
Pull request #463 (2019-12-26 22:34:03)

Add two new printer options: -knl and -bnl
Hi,

I have added two new printer options for a less compact style.

`-knl` (_keyword new line_) will put keywords like "then" and "do" on a new line.

**Example:**
```
if foo
then
    bar
fi
```
`-bnl` (_brace new line_) will put braces on new lines.

**Example:**
```
foo()
{
    bar
}
```
It would be great if you would accept the patch. Let me know if there is anything I need to change.

Regards,
Morgan

mvdan: Thanks for the patch. It's clear that you spent some effort here, which puts me in a difficult position because this feature was brought up and rejected in #168.

In the future, I'd strongly encourage you to raise an issue or talk to upstream developers before writing new features, especially if they mean new APIs or command line flags. Those changes tend to be delicate and require careful consideration, so jumping straight to a patch forces upstream's hand to merge or reject.

For now, I'll reopen the issue, as there was another person interested in this feature recently. And as said before, I don't think I'll be merging this PR until there's at least some consensus in #168.
MorganAntonsson: No worries. I should have done some research before submitting the code. Thanks for re-opening the issue.
mvdan: Could you please trim down this PR to follow option 2 in https://github.com/mvdan/sh/issues/229#issuecomment-575877030? That is, `-fn` to keep function declaration opening braces in a separate line.

We also need more test cases - for example, inputs like the following (imagine tabs for indentation):

```
foo()

{

    bar
}

function foo {
    bar
}

foo()
{ bar; }
```
MorganAntonsson: Sure, it's on my todo-list. I am a bit busy at the moment, but within a week or two I should have time.
MorganAntonsson: Hi. I have made the update. Let me know if it's OK. :)
MorganAntonsson: Updated. Sure, feel free to edit the commit message. Thanks for accepting the change!

Patch:
diff --git a/cmd/shfmt/main.go b/cmd/shfmt/main.go
index 59f0ee5a..fbaae1fb 100644
--- a/cmd/shfmt/main.go
+++ b/cmd/shfmt/main.go
@@ -38,11 +38,12 @@ var (
 	langStr = flag.String("ln", "", "")
 	posix   = flag.Bool("p", false, "")
 
-	indent      = flag.Uint("i", 0, "")
-	binNext     = flag.Bool("bn", false, "")
-	caseIndent  = flag.Bool("ci", false, "")
-	spaceRedirs = flag.Bool("sr", false, "")
-	keepPadding = flag.Bool("kp", false, "")
+	indent          = flag.Uint("i", 0, "")
+	binNext         = flag.Bool("bn", false, "")
+	caseIndent      = flag.Bool("ci", false, "")
+	spaceRedirs     = flag.Bool("sr", false, "")
+	keepPadding     = flag.Bool("kp", false, "")
+	functionNewLine = flag.Bool("fn", false, "")
 
 	toJSON = flag.Bool("tojson", false, "")
 
@@ -91,6 +92,7 @@ Printer options:
   -ci       switch cases will be indented
   -sr       redirect operators will be followed by a space
   -kp       keep column alignment paddings
+  -fn       function opening braces are placed on a separate line
 
 Utilities:
 
@@ -116,7 +118,7 @@ Utilities:
 	}
 	flag.Visit(func(f *flag.Flag) {
 		switch f.Name {
-		case "ln", "p", "i", "bn", "ci", "sr", "kp":
+		case "ln", "p", "i", "bn", "ci", "sr", "kp", "fn":
 			useEditorConfig = false
 		}
 	})
@@ -145,6 +147,7 @@ Utilities:
 		syntax.SwitchCaseIndent(*caseIndent)(printer)
 		syntax.SpaceRedirects(*spaceRedirs)(printer)
 		syntax.KeepPadding(*keepPadding)(printer)
+		syntax.FunctionNewLine(*functionNewLine)(printer)
 	}
 
 	if os.Getenv("FORCE_COLOR") == "true" {
diff --git a/syntax/printer.go b/syntax/printer.go
index 8ab1bdcd..37cfce26 100644
--- a/syntax/printer.go
+++ b/syntax/printer.go
@@ -68,6 +68,11 @@ func Minify(enabled bool) PrinterOption {
 	return func(p *Printer) { p.minify = enabled }
 }
 
+// FunctionNewLine will place function braces on a new line.
+func FunctionNewLine(enabled bool) PrinterOption {
+	return func(p *Printer) { p.functionNewLine = enabled }
+}
+
 // NewPrinter allocates a new Printer and applies any number of options.
 func NewPrinter(opts ...PrinterOption) *Printer {
 	p := &Printer{
@@ -188,12 +193,13 @@ type Printer struct {
 	tabWriter *tabwriter.Writer
 	cols      colCounter
 
-	indentSpaces   uint
-	binNextLine    bool
-	swtCaseIndent  bool
-	spaceRedirects bool
-	keepPadding    bool
-	minify         bool
+	indentSpaces    uint
+	binNextLine     bool
+	swtCaseIndent   bool
+	spaceRedirects  bool
+	keepPadding     bool
+	minify          bool
+	functionNewLine bool
 
 	wantSpace   bool
 	wantNewline bool
@@ -962,6 +968,8 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) {
 	case *Block:
 		p.WriteByte('{')
 		p.wantSpace = true
+		// Forbid "foo()\n{ bar; }"
+		p.wantNewline = p.wantNewline || p.functionNewLine
 		p.nestedStmts(x.Stmts, x.Last, x.Rbrace)
 		p.semiRsrv("}", x.Rbrace)
 	case *IfClause:
@@ -1040,7 +1048,9 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) {
 		}
 		p.writeLit(x.Name.Value)
 		p.WriteString("()")
-		if !p.minify {
+		if p.functionNewLine {
+			p.newline(Pos{})
+		} else if !p.minify {
 			p.space()
 		}
 		p.line = x.Body.Pos().Line()


Test patch:
diff --git a/syntax/printer_test.go b/syntax/printer_test.go
index c64bdd82..556b63ea 100644
--- a/syntax/printer_test.go
+++ b/syntax/printer_test.go
@@ -743,6 +743,35 @@ func TestPrintSwitchCaseIndent(t *testing.T) {
 	}
 }
 
+func TestPrintFunctionNewLine(t *testing.T) {
+	t.Parallel()
+	tests := [...]printCase{
+		{
+			"foo() { bar; }",
+			"foo()\n{\n\tbar\n}",
+		},
+		{
+			"foo()\n{ bar; }",
+			"foo()\n{\n\tbar\n}",
+		},
+		{
+			"foo()\n\n{\n\n\tbar\n}",
+			"foo()\n{\n\n\tbar\n}",
+		},
+		{
+			"function foo {\n\tbar\n}",
+			"function foo()\n{\n\tbar\n}",
+		},
+	}
+	parser := NewParser(KeepComments(true))
+	printer := NewPrinter(FunctionNewLine(true))
+	for i, tc := range tests {
+		t.Run(fmt.Sprintf("%03d", i), func(t *testing.T) {
+			printTest(t, parser, printer, tc.in, tc.want)
+		})
+	}
+}
+
 func TestPrintSpaceRedirects(t *testing.T) {
 	t.Parallel()
 	tests := [...]printCase{