amazon-ion / ion-intellij-plugin

Support for Ion in Intellij IDEA.
Apache License 2.0
29 stars 22 forks source link

Fixes SExpression formatting #65

Closed desaikd closed 3 months ago

desaikd commented 3 months ago

Issue #64

Description of changes: This PR fixes SExpression formatting

Example: Sample Ion data(without any formatting):

{
  foo: "This is Foo",
  bar: (bar { bar: "This is bar" } "bar"),
  baz: [(baz "baz"), BAZ, Baz]
}

Before the fix(After running code formatter from IDE):

{
  foo: "This is Foo",
  bar: (bar { // this struct should be on next line
    bar: "This is bar"
  } "bar"
  ),
  baz: [
    (baz "baz" // "baz" should be on next line and indented
    ),
    BAZ,
    Baz
  ]
}

After the fix(After running code formatter from IDE):

{
  foo: "This is Foo",
  bar: (
    bar
    {
      bar: "This is bar"
    }
    "bar"
  ),
  baz: [
    (
      baz
      "baz"
    ),
    BAZ,
    Baz
  ]
}

List of changes:

Test: Tested with gradle build task that runs all the tests. Also ran buildPlugin and runIde tasks.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

popematt commented 3 months ago

Thanks for tackling this!

Regarding Removes treating first element as operator in SExp, there are some people who like to use lisp-style formatting for SExpressions in Ion. Could we keep it, but make it conditional based on some setting? Perhaps using something like this?

toddjonker commented 3 months ago

I don't understand the goal of this PR. While not ideal, the current format-file behavior is better than the proposed change.

S-expressions are intended to be used for code and should follow Lisp-like formatting by default. There's a lot one could say about the topic, but at minimum it should keep the first child element adjacent to the opening paren, and any line-breaks inside should have at least one level of indentation relative to the opening paren.

desaikd commented 3 months ago

As per the review comments and the original implementation that used lisp style formatting, I am reverting the previous commit https://github.com/amazon-ion/ion-intellij-plugin/pull/65/commits/c489a4ed70fb6807c4ad3407efc1240caf2cb23e. This PR still fixes some of the indentation and line break issue as described in Before fix example of the PR description. I have also created an issue to look into making an optional setting for sexpression formatting either Lisp style(default) or similar to list formatting.

As per the current implementation following is the After fix example:

{
  foo: "This is Foo",
  bar: (bar
    {
      bar: "This is bar"
    }
    "bar"
  ),
  baz: [
    (baz
      "baz"
    ),
    BAZ,
    Baz
  ]
}