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

Bugfix: collection literals indentation in formatter #3959

Conversation

juanedi
Copy link
Contributor

@juanedi juanedi commented Jan 30, 2017

Fixes #3915

Changed formatter indentation for "collection" literals (arrays, hashes, tuples, ...)

When the expression doesn't begin with a newline, contents are now aligned at the same column as the starting token, so this:

x = {    foo: 1,
              bar: 2 }

... turns into this:

    x = {foo: 1,
         bar: 2}

When a newline is present, keep indentation as before (ie. indent one level deeper). This means this:

    x = {
        foo: 1,
      bar: 2
    }

... turns into this:

    x = {
      foo: 1,
      bar: 2,
    }

The reason for this behaviour is not to impose very long indentation offsets in expressions such as this one:

my_descriptive_variable_name = {
  foo: 1,
  bar: 2
}

The only odd case would be this one:

x = {foo: 1,
     bar: 2,
}

... but I guess one could argue that such writing is inconsistent and we should either choose between newline at both the beginning and end of the expression or no newline at at. Enforcing this would be a bigger change.

Here is a summary of the specs I needed to change:


Before:

[1,
  2,
  3]

After:

[1,
 2,
 3]

Before:

[1,
  2,
  3,
]

After:

[1,
 2,
 3,
]

Before:

[1,
  2,
  3]

After:

[1,
 2,
 3]

Before:

{1 => 2,
  3 => 4}

After:

{1 => 2,
 3 => 4}

Before:

{1 => 2,

  3 => 4}

After:

{1 => 2,

 3 => 4}

Before:

[1, 2,
  3, 4]

After:

[1, 2,
 3, 4]

Before:

{1 => 2,
  3 => 4, # lala
}

After:

{1 => 2,
 3 => 4, # lala
}

@juanedi juanedi changed the title Bufgix/formatter literal elements indentation Bugfix: collection literals indentation in formatter Jan 30, 2017
"hahah⊙ⓧ⊙" => "aGFoYWjiipnik6fiipk=\n"}
"Now is the time for all good coders\nto learn Crystal" => "Tm93IGlzIHRoZSB0aW1lIGZvciBhbGwgZ29vZCBjb2RlcnMKdG8gbGVhcm4g\nQ3J5c3RhbA==\n",
"This is line one\nThis is line two\nThis is line three\nAnd so on...\n" => "VGhpcyBpcyBsaW5lIG9uZQpUaGlzIGlzIGxpbmUgdHdvClRoaXMgaXMgbGlu\nZSB0aHJlZQpBbmQgc28gb24uLi4K\n",
"hahah⊙ⓧ⊙" => "aGFoYWjiipnik6fiipk=\n"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like here => one space righter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this formatter modifications just care about the alignment of the beginning of the expressions. The arrows there are not perfectly aligned just because the code before running the formatter was not perfectly aligned :-)

If you were suggesting to change this particular line in order to align the arrows, I'll be glad to do it. On the other hand, I'm not sure the formatter should be in charge of aligning the arrows. That should probably be discussed in another ticket.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK it's because of variable-width encoding of utf-8 standard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes in code this looks ok, this is just github formatting

@asterite
Copy link
Member

asterite commented Feb 8, 2017

This looks really good to me! So 👍 for merging it.

@bcardiff
Copy link
Member

bcardiff commented Feb 9, 2017

@juanedi could you rebase on master so the ci can check that the latest changes are still formatted accordingly to this PR?

When the collection literal doesn't begin with a newline, contents are now aligned at the same column as the starting token, so this:

x = { foo: 1,
  bar: 2 }

... turns into this:

x = {foo: 1,
     bar: 2}

When a newline is present, keep indentation as before (ie. indent one level deeper). This means this:

x = {
    foo: 1,
  bar: 2
}

... turns into this:

x = {
  foo: 1,
  bar: 2,
}
@juanedi juanedi force-pushed the bufgix/formatter-literal-elements-indentation branch from b38d686 to bd1330d Compare February 10, 2017 13:13
@spalladino spalladino merged commit dff9b2e into crystal-lang:master Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants