Fix heredoc start + comma formatter #6222

straight-shoota · 2018-06-19T17:23:13Z

Fixes #6197

This is not a great fix, but it seems to work. If anyone has a better solution, go ahead.

asterite · 2018-06-20T16:46:56Z

I think the bug is in format_literal_elements, but the code is way to complex by now to fix it in another way...

bcardiff · 2018-06-21T08:14:15Z

I would ask @makenowjust if there is another solution that can be thought, but otherwise LGTM.

Can we add a spec with more than one hash entry with heredocs?

straight-shoota · 2018-06-21T09:01:51Z

@asterite Thanks for the advice. It helped a lot 👍

The complexity is not even that much of a problem. I got it to work (mostly) and format_literal_elements certainly a much better place to fix this.
I'm still left with a rather strange indent on the last line, I can't really figure why that is inserted at in finish_list when @wrote_newline is true. It seems only to be an issue when heredocs are involved, but not with a quoted string literal for example.

makenowjust · 2018-06-21T09:08:28Z

At least this implementation is not complete. Example:

{
  :foo => <<-FOO ,
          foo
          FOO
}

is formatted to

{
  :foo => <<-FOO
          foo foo
          FOO
,
  }

I agree with @asterite and such a specialization for heredoc is already used in format_args_simple (at line 2483) for example. I'll try to fix this.

makenowjust · 2018-06-21T09:20:10Z

Above comment is about old commit. And a new version looks good to me. Sorry 🙇‍♂️ -- 木曜日, 21 6月 2018, 06:02午後 +09:00 from Johannes Müller notifications@github.com :

…

@asterite Thanks for the advice. It helped a lot 👍 The complexity is not even that much of a problem. I got it to work (mostly) and format_literal_elements certainly a much better place to fix this. I'm still left with a rather strange indent on the last line, I can't really figure why that is inserted at in finish_list when @wrote_newline is true. It seems only to be an issue when heredocs are involved, but not with a quoted string literal for example. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub , or mute the thread .

makenowjust · 2018-06-21T10:17:17Z

@straight-shoota When @wrote_newline is true, it means newline is wrote already but indent is not wrote. So inserting newline here is correct in general.

makenowjust · 2018-06-21T10:20:05Z

src/compiler/crystal/tools/formatter.cr

@@ -828,17 +828,23 @@ module Crystal
          accept element
        end

+        element_is_heredoc = !@lexer.heredocs.empty?


I think this variable name is not correct. It should be named has_heredoc_in_line or has_heredoc as short.

makenowjust · 2018-06-21T10:23:32Z

src/compiler/crystal/tools/formatter.cr

-          write "," unless last || found_comment
+          if !found_comment && (!last || element_is_heredoc)
+            write ","
+            wrote_comma = true


wrote_comma is assigned here, but it is not reset. So this affects other iterations.

The variable is limited to block scope and won't be copied over between iterations.

Yes. Sorry.

straight-shoota · 2018-06-21T10:41:12Z

@makenowjust Yes, writing an indent is correct. But it seems that @indent should be 0 at that point, yet it is actually still 2.

[
  <<-EOF,
  foo
  EOF
  ]

# without comma:
[
  <<-EOF
  foo
  EOF
]

# quoted string literal
[
  "foo",
]

makenowjust · 2018-06-21T10:45:39Z

@straight-shoota I think this patch fixes indent issue:

diff --git a/src/compiler/crystal/tools/formatter.cr b/src/compiler/crystal/tools/formatter.cr
index 29d13c5c0..3fb1f89c3 100644
--- a/src/compiler/crystal/tools/formatter.cr
+++ b/src/compiler/crystal/tools/formatter.cr
@@ -623,7 +623,7 @@ module Crystal
       next_token

       @string_continuation = old_string_continuation
-      @indent = old_indent
+      @indent = old_indent unless is_heredoc

       false
     end

straight-shoota · 2018-06-21T10:50:25Z

Awesome! Thank you very much @makenowjust
I had been looking for places where @indent was changed, but it seems I missed that spot.

asterite · 2018-06-21T12:13:47Z

Also, feel free to redactor/improvr/destroy the current formatter for better readability. It started simple but grew complex, mainly because the syntax of the language is so wide and complex too.

bcardiff

What a nice collaboration we end up having here! 💚

Thanks @makenowjust for jumping in and @straight-shoota for dealing with the issue.

makenowjust · 2018-06-21T16:51:08Z

spec/compiler/formatter/formatter_spec.cr

+  assert_format "[\n  <<-EOF,\n  foo\n  EOF\n  <<-BAR,\n  bar\n  BAR\n]"
+  assert_format "Hash{\n  foo => <<-EOF,\n  foo\n  EOF\n}"
+  assert_format "Hash{\n  foo => <<-EOF,\n  foo\n  EOF\n  bar => <<-BAR,\n  bar\n  BAR\n}"
+  assert_format "Hash{\n  foo => <<-EOF\n  foo\n  EOF\n}"


Is this really right? I think it should append comma. So correct assertion is:

assert_format "Hash{\n foo => <<-EOF\n foo\n EOF\n}", "Hash{\n foo => <<-EOF,\n foo\n EOF\n}"

Because this code:

{ foo: 42 }

is formatted to:

{ foo: 42, }

Yeah, that's probably better. But it seems a bit more difficult because of the special treatment of newline in next_token. And it requires changes in a some other examples, that are - currently - explicitly not adding a comma there:

https://github.com/straight-shoota/crystal/blob/4c63cd398fdf2f98ee05a6e12506e979d692c5ab/spec/compiler/formatter/formatter_spec.cr#L1135-L1138

I'd suggest to leave this PR as a fix for the immediate issue.

I agree with @straight-shoota here. Not adding a comma although is inconsistent with non heredoc hash/array literals, at least is not producing invalid code.

@sdogruyol @makenowjust WDYT?

I think, this is OK 👍

RX14 · 2018-06-25T09:28:56Z

Would greatly appreciate a fix to not appending a comma, or at least an issue so we can pretend we haven't forgotten about it.

bcardiff · 2018-06-25T09:32:18Z

@RX14 precisely this PR does not append a comma in multiline hashes with heredocs, what is missing is the comma to be consistent with other hashes

RX14 · 2018-06-25T17:21:50Z

@bcardiff I understood that, I just worded it weirdly, should have said fix for instead of fix to.

bcardiff added this to the 0.25.1 milestone Jun 21, 2018

makenowjust reviewed Jun 21, 2018

View reviewed changes

makenowjust suggested changes Jun 21, 2018

View reviewed changes

makenowjust approved these changes Jun 21, 2018

View reviewed changes

straight-shoota force-pushed the jm/fix/6197 branch from c710905 to 7b17907 Compare June 21, 2018 10:55

wooster0 approved these changes Jun 21, 2018

View reviewed changes

sdogruyol approved these changes Jun 21, 2018

View reviewed changes

Fix heredoc start + comma formatter

4c63cd3

straight-shoota force-pushed the jm/fix/6197 branch from 7b17907 to 4c63cd3 Compare June 21, 2018 12:39

bcardiff approved these changes Jun 21, 2018

View reviewed changes

makenowjust suggested changes Jun 21, 2018

View reviewed changes

makenowjust approved these changes Jun 22, 2018

View reviewed changes

RX14 added kind:bug topic:compiler topic:tools:formatter topic:tools labels Jun 25, 2018

RX14 merged commit 3e794db into crystal-lang:master Jun 25, 2018

straight-shoota deleted the jm/fix/6197 branch June 25, 2018 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix heredoc start + comma formatter #6222

Fix heredoc start + comma formatter #6222

straight-shoota commented Jun 19, 2018

asterite commented Jun 20, 2018

bcardiff commented Jun 21, 2018

straight-shoota commented Jun 21, 2018

makenowjust commented Jun 21, 2018

makenowjust commented Jun 21, 2018 via email

makenowjust commented Jun 21, 2018

makenowjust Jun 21, 2018

makenowjust Jun 21, 2018 •

edited

straight-shoota Jun 21, 2018

makenowjust Jun 21, 2018

straight-shoota commented Jun 21, 2018

makenowjust commented Jun 21, 2018

straight-shoota commented Jun 21, 2018

asterite commented Jun 21, 2018

bcardiff left a comment

makenowjust Jun 21, 2018

straight-shoota Jun 21, 2018

bcardiff Jun 22, 2018

makenowjust Jun 22, 2018

sdogruyol Jun 22, 2018

RX14 commented Jun 25, 2018 •

edited

bcardiff commented Jun 25, 2018

RX14 commented Jun 25, 2018

Fix heredoc start + comma formatter #6222

Fix heredoc start + comma formatter #6222

Conversation

straight-shoota commented Jun 19, 2018

asterite commented Jun 20, 2018

bcardiff commented Jun 21, 2018

straight-shoota commented Jun 21, 2018

makenowjust commented Jun 21, 2018

makenowjust commented Jun 21, 2018 via email

makenowjust commented Jun 21, 2018

Choose a reason for hiding this comment

makenowjust Jun 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

straight-shoota commented Jun 21, 2018

makenowjust commented Jun 21, 2018

straight-shoota commented Jun 21, 2018

asterite commented Jun 21, 2018

bcardiff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RX14 commented Jun 25, 2018 • edited

bcardiff commented Jun 25, 2018

RX14 commented Jun 25, 2018

makenowjust Jun 21, 2018 •

edited

RX14 commented Jun 25, 2018 •

edited