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

Fixes SExpression formatting #65

Merged
merged 5 commits into from
Jun 11, 2024
Merged

Fixes SExpression formatting #65

merged 5 commits into from
Jun 11, 2024

Conversation

desaikd
Copy link
Contributor

@desaikd desaikd commented May 23, 2024

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.

…LUE`

* Adds changes in `IonSExpressionBlock` to consider `SEXPRESSION_ATOM`
as children as per BNF grammar
* Adds changes in `IonCodeBlockSpacing` to correctly consider
`SEXPRESSION_ATOM` as children as per BNF grammar
@desaikd desaikd force-pushed the desaikd-sexpression-issue branch from 17b7b90 to f0b4f52 Compare May 23, 2024 22:42
* Removed operator definition and usage in SExp from grammar
* All the code in gen/ is generated from IDE `generate parser` based on
BNF grmmar
* Removed all reference of sexpression operator
* Modified test to format first element as a normal sexpression element
@popematt
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

desaikd commented Jun 10, 2024

As per the review comments and the original implementation that used lisp style formatting, I am reverting the previous commit c489a4e. 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
  ]
}

@desaikd desaikd merged commit 36f8093 into master Jun 11, 2024
6 checks passed
peddiashrith pushed a commit to peddiashrith/ion-intellij-plugin that referenced this pull request Sep 13, 2024
* Changes SExpression children to use `SEXPRESSION_ATOM` instead of `VALUE`
* Adds changes in `IonSExpressionBlock` to consider `SEXPRESSION_ATOM`
as children as per BNF grammar
* Adds changes in `IonCodeBlockSpacing` to correctly consider
`SEXPRESSION_ATOM` as children as per BNF grammar
* Adds changes in tests for usign correct node names
* Adds changes for PR workflow to run Gradle Build task
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.

4 participants