Allow to format method arguments with 2 spaces indentation #4980

makenowjust · 2017-09-16T04:42:28Z

Fixed #1924

Now, this code:

def foo(
        a : Int32, b : String,
        c)
end

is formatted to:

def foo(
  a : Int32, b : String,
  c
)
end

/cc @miketheman

Fixed crystal-lang#1924

asterite · 2017-09-16T13:53:44Z

@makenowjust Thank you so much for this!

Right now the formatter's code is very messy. I'd like to improve it, the second time I wrote a formatter the end result was much cleaner. If I have time I'll improve the current code.

For now, I tried to implement what you did but with less code. Here's my diff:

diff --git a/spec/compiler/formatter/formatter_spec.cr b/spec/compiler/formatter/formatter_spec.cr
index 6e748e714..25091f61a 100644
--- a/spec/compiler/formatter/formatter_spec.cr
+++ b/spec/compiler/formatter/formatter_spec.cr
@@ -157,7 +157,8 @@ describe Crystal::Formatter do
   assert_format "def   foo (  x , y , )  \n  end", "def foo(x, y)\nend"
   assert_format "def   foo (  x , y ,\n)  \n  end", "def foo(x, y)\nend"
   assert_format "def   foo (  x ,\n y )  \n  end", "def foo(x,\n        y)\nend"
-  assert_format "def   foo (\nx ,\n y )  \n  end", "def foo(\n        x,\n        y)\nend"
+  assert_format "def   foo (\nx ,\n y )  \n  end", "def foo(\n  x,\n  y\n)\nend"
+  assert_format "class Foo\ndef   foo (\nx ,\n y )  \n  end\nend", "class Foo\n  def foo(\n    x,\n    y\n  )\n  end\nend"
   assert_format "def   foo (  @x)  \n  end", "def foo(@x)\nend"
   assert_format "def   foo (  @x, @y)  \n  end", "def foo(@x, @y)\nend"
   assert_format "def   foo (  @@x)  \n  end", "def foo(@@x)\nend"
@@ -581,6 +582,7 @@ describe Crystal::Formatter do
   assert_format "lib Foo\nfun foo(x : Int32, ... )  :  Int32\nend", "lib Foo\n  fun foo(x : Int32, ...) : Int32\nend"
   assert_format "lib Foo\n  fun foo(Int32) : Int32\nend"
   assert_format "fun foo(x : Int32) : Int32\n  1\nend"
+  assert_format "fun foo(\n  x : Int32,\n  ...\n) : Int32\n  1\nend"
   assert_format "lib Foo\n  fun foo = bar(Int32) : Int32\nend"
   assert_format "lib Foo\n  fun foo = \"bar\"(Int32) : Int32\nend"
   assert_format "lib Foo\n  $foo  :  Int32 \nend", "lib Foo\n  $foo : Int32\nend"
@@ -742,8 +744,8 @@ describe Crystal::Formatter do
   assert_format "foo(\n# x\n1,\n\n# y\nz: 2\n)", "foo(\n  # x\n  1,\n\n  # y\n  z: 2\n)"
   assert_format "foo(\n# x\n1,\n\n# y\nz: 2,\n\n# a\nb: 3)", "foo(\n  # x\n  1,\n\n  # y\n  z: 2,\n\n  # a\n  b: 3)"
   assert_format "foo(\n 1, # hola\n2, # chau\n )", "foo(\n  1, # hola\n  2, # chau\n)"
-  assert_format "def foo(\n\n#foo\nx,\n\n#bar\nz\n)\nend", "def foo(\n        # foo\n        x,\n\n        # bar\n        z)\nend"
-  assert_format "def foo(\nx, #foo\nz #bar\n)\nend", "def foo(\n        x, # foo\n        z  # bar\n        )\nend"
+  assert_format "def foo(\n\n#foo\nx,\n\n#bar\nz\n)\nend", "def foo(\n  # foo\n  x,\n\n  # bar\n  z\n)\nend"
+  assert_format "def foo(\nx, #foo\nz #bar\n)\nend", "def foo(\n  x, # foo\n  z  # bar\n)\nend"
   assert_format "a = 1;;; b = 2", "a = 1; b = 2"
   assert_format "a = 1\n;\nb = 2", "a = 1\nb = 2"
   assert_format "foo do\n  # bar\nend"
@@ -1019,7 +1021,7 @@ describe Crystal::Formatter do
   assert_format "foo { |x| (x).a }"
   assert_format "def foo(\n        &block)\nend"
   assert_format "def foo(a,\n        &block)\nend"
-  assert_format "def foo(\n        a,\n        &block)\nend"
+  assert_format "def foo(\n  a,\n  &block\n)\nend"
   assert_format "def foo(a,\n        *b)\nend"
   assert_format "def foo(a,\n        **b)\nend"
   assert_format "def foo(**b, # comment\n        &block)\nend"
diff --git a/src/compiler/crystal/tools/formatter.cr b/src/compiler/crystal/tools/formatter.cr
index 3ecc58b03..f6486c364 100644
--- a/src/compiler/crystal/tools/formatter.cr
+++ b/src/compiler/crystal/tools/formatter.cr
@@ -1447,6 +1447,7 @@ module Crystal
         next_needs_indent = false
         has_parentheses = false
         found_comment = false
+        needs_end_newline = false
 
         if @token.type == :"("
           has_parentheses = true
@@ -1454,8 +1455,10 @@ module Crystal
           next_token_skip_space
           if @token.type == :NEWLINE
             write_line
-            skip_space_or_newline(prefix_size)
+            prefix_size = old_indent + 2
             next_needs_indent = true
+            needs_end_newline = true
+            skip_space_or_newline(prefix_size)
           end
           skip_space_or_newline
         else
@@ -1484,7 +1487,7 @@ module Crystal
           skip_space
 
           if @token.type == :","
-            has_more = !last?(i, args) || double_splat || block_arg
+            has_more = !last?(i, args) || double_splat || block_arg || variadic
 
             if has_more
               write ","
@@ -1557,13 +1560,27 @@ module Crystal
         end
 
         if variadic
-          write_token ", ", :"..."
+          if next_needs_indent
+            write_indent(prefix_size)
+            next_needs_indent = false
+          end
+          unless comma_written
+            write ", "
+            comma_written = true
+          end
+          write_token :"..."
           skip_space_or_newline
         end
 
         if has_parentheses
-          skip_space_or_newline
-          write_indent(prefix_size) if found_comment
+          found_comment ||= skip_space_or_newline
+          if needs_end_newline && !found_comment
+            write_line
+            write_indent(old_indent)
+          elsif !needs_end_newline && found_comment
+            write_indent(prefix_size)
+          end
+
           write_token :")"
         else
           write_indent(prefix_size) if found_comment
diff --git a/src/compiler/crystal/tools/init.cr b/src/compiler/crystal/tools/init.cr
index 0ab7f0504..8759f761d 100644
--- a/src/compiler/crystal/tools/init.cr
+++ b/src/compiler/crystal/tools/init.cr
@@ -102,13 +102,14 @@ module Crystal
       property silent : Bool
 
       def initialize(
-                     @skeleton_type = "none",
-                     @name = "none",
-                     @dir = "none",
-                     @author = "none",
-                     @email = "none",
-                     @github_name = "none",
-                     @silent = false)
+        @skeleton_type = "none",
+        @name = "none",
+        @dir = "none",
+        @author = "none",
+        @email = "none",
+        @github_name = "none",
+        @silent = false
+      )
       end
     end
 
diff --git a/src/llvm/lib_llvm_ext.cr b/src/llvm/lib_llvm_ext.cr
index 953567eb8..56d650f11 100644
--- a/src/llvm/lib_llvm_ext.cr
+++ b/src/llvm/lib_llvm_ext.cr
@@ -14,10 +14,11 @@ lib LibLLVMExt
   fun di_builder_finalize = LLVMDIBuilderFinalize(DIBuilder)
 
   fun di_builder_create_function = LLVMDIBuilderCreateFunction(
-                                                               builder : DIBuilder, scope : Metadata, name : Char*,
-                                                               linkage_name : Char*, file : Metadata, line : UInt,
-                                                               composite_type : Metadata, is_local_to_unit : Bool, is_definition : Bool,
-                                                               scope_line : UInt, flags : LLVM::DIFlags, is_optimized : Bool, func : LibLLVM::ValueRef) : Metadata
+    builder : DIBuilder, scope : Metadata, name : Char*,
+    linkage_name : Char*, file : Metadata, line : UInt,
+    composite_type : Metadata, is_local_to_unit : Bool, is_definition : Bool,
+    scope_line : UInt, flags : LLVM::DIFlags, is_optimized : Bool, func : LibLLVM::ValueRef
+  ) : Metadata
 
   fun di_builder_create_file = LLVMDIBuilderCreateFile(builder : DIBuilder, file : Char*, dir : Char*) : Metadata
   fun di_builder_create_compile_unit = LLVMDIBuilderCreateCompileUnit(builder : DIBuilder,

I'd use my diff, only because it introduces less changes and it's simpler. I have to say I peeked at your code a couple of times to be able to do it, so your PR was very helpful! I also added an extra spec to make sure the ending ")" is correctly indented.

This seems a bit weird at first:

def foo(
  x,
  y
)
end

But I think it helps to quickly notice when method arguments end and the body starts:

def foo(
  x,
  y
)
  puts x + y
end

So 👍 for this (with my diff ;-))

miketheman · 2017-09-16T14:34:38Z

Thanks to you both for sorting this out! I'm happy with either solution.

mverzilli · 2017-09-16T21:40:14Z

@makenowjust do you want to incorporate @asterite 's diff to this PR?

makenowjust · 2017-09-17T04:13:09Z

@mverzilli I'll try it. I think @asterite's diff is totally good, but I'm dubious about these lines:

-          skip_space_or_newline
-          write_indent(prefix_size) if found_comment
+          found_comment ||= skip_space_or_newline
+          if needs_end_newline && !found_comment
+            write_line
+            write_indent(old_indent)
+          elsif !needs_end_newline && found_comment
+            write_indent(prefix_size)
+          end
+

I'll fix them up and write more detailed specs. Thank you @asterite!

Fix crystal-lang#1924 Close crystal-lang#4980, crystal-lang#4990 This is just refactoring but it contains many bug fixes and improvements. These come from refactoring effects, so I cannot separate them from this. Bug fixes and improvements: - work formatting double splat with trailing comma (crystal-lang#4990) - work formatting when there is newline or comment between argument and comma. ```crystal def foo(a ,) end def foo(a # comment ,) end ``` is formatted to correctly: ```crystal def foo(a) end def foo(a # comment ) end ``` - format arguments with 2 spaces indentation (crystal-lang#1924) - distinguish comment followed on newline and argument. The formatter keeps this for example: ```crystal def foo(a # argument 'a' is ... ) end def foo(a # more argument here in future... ) end ``` And, perhaps there are other bug fixes and improvements because `format_def_args` has too many bugs to remember. Refactoring is so great!!

Fix #1924 Close #4980, #4990 This is just refactoring but it contains many bug fixes and improvements. These come from refactoring effects, so I cannot separate them from this. Bug fixes and improvements: - work formatting double splat with trailing comma (#4990) - work formatting when there is newline or comment between argument and comma. ```crystal def foo(a ,) end def foo(a # comment ,) end ``` is formatted to correctly: ```crystal def foo(a) end def foo(a # comment ) end ``` - format arguments with 2 spaces indentation (#1924) - distinguish comment followed on newline and argument. The formatter keeps this for example: ```crystal def foo(a # argument 'a' is ... ) end def foo(a # more argument here in future... ) end ``` And, perhaps there are other bug fixes and improvements because `format_def_args` has too many bugs to remember. Refactoring is so great!! * Rename 'wrote_newline' inside 'format_def_arg' to 'just_wrote_newline' And fix some conditions for readbility.

Allow to format method arguments with 2 spaces indentation

f7175d1

Fixed crystal-lang#1924

mverzilli added kind:feature topic:tools:formatter labels Sep 16, 2017

makenowjust mentioned this pull request Sep 17, 2017

Refactor 'Crystal::Formatter#format_def_args' #4992

Merged

mverzilli closed this in #4992 Sep 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to format method arguments with 2 spaces indentation #4980

Allow to format method arguments with 2 spaces indentation #4980

makenowjust commented Sep 16, 2017

asterite commented Sep 16, 2017

miketheman commented Sep 16, 2017

mverzilli commented Sep 16, 2017

makenowjust commented Sep 17, 2017

Allow to format method arguments with 2 spaces indentation #4980

Allow to format method arguments with 2 spaces indentation #4980

Conversation

makenowjust commented Sep 16, 2017

asterite commented Sep 16, 2017

miketheman commented Sep 16, 2017

mverzilli commented Sep 16, 2017

makenowjust commented Sep 17, 2017