Allow to format method arguments with 2 spaces indentation #4980
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@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
)
endBut I think it helps to quickly notice when method arguments end and the body starts: def foo(
x,
y
)
puts x + y
endSo 👍 for this (with my diff ;-)) |
|
Thanks to you both for sorting this out! I'm happy with either solution. |
|
@makenowjust do you want to incorporate @asterite 's diff to this PR? |
|
@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! |
makenowjust
added a commit
to makenowjust/crystal
that referenced
this pull request
Sep 17, 2017
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!!
mverzilli
pushed a commit
that referenced
this pull request
Sep 17, 2017
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed #1924
Now, this code:
is formatted to:
/cc @miketheman