Skip to content

Improve List/Map Formatting #478

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

Open
wants to merge 38 commits into
base: main
Choose a base branch
from

Conversation

simonthuresson
Copy link
Contributor

@simonthuresson simonthuresson commented Apr 22, 2025

Description

This PR improves how List:s, Map:s and Map Projections are being formatted in certain cases. This was a tech debt from #460.
The general idea of this PR is to fix the following:

// Before
RETURN 
  [
    item1,
    item2, 
    item3
  ]
// After
RETURN [
  item1,
  item2, 
  item3
]

There are a few caveats to this. This type of formatting only occurs if the List/Map is "alone". Meaning if two lists would occur in the return, the regular formatting applies. Which is similar to how prettier does it:

RETURN 
  [
    item1,
    item2, 
    item3
  ],
  [
    item4,
    item5, 
    item6
  ]

Another case is if a comment appear, then the regular style is also applied. This is because otherwise the comment will end up inside the list, which may cause the comment to refer to something else.
This is how we do it:

RETURN // A Comment in weird place
  [
    item1,
    item2, 
    item3
  ]

This PR covers the cases where we know so far should have the special split. There is a chance that later on more places should have special split.

Special Split

These things can special split:

  • Map
  • Map projection
  • List

In these places:

  • UNWIND
  • Function invocation
  • RETURN statement
  • REDUCE expression
  • String and List comparison

So to allow a special split, two things needs to be fullfilled. A parent should only have one child, and that child should want to special split. This means that if a return has two lists as return expression, the first part is not fulfilled of only having one child. Similarly, if a return expression contains a node expression for example, the child is not of "special split" kind, and therefore not fulfilling the second part.

Indentation

The most challening thing about this PR was how to handle indentation. In the regular case, both the return statement and the list adds one level of indentation. However, with the "special" formatting, only one should be applied. If you just ignore adding of the the indentation levels, the formatter throws an error. This is by design to not allow removing indentation levels which has not been applied. The solution to this was to let the add indentation part include a reference to the remove indentation modifier, if the add indentation is not applied, a flag is set on the remove indentation.

Tests

  • About 10 tests has been modified to suit the new formatting style
  • About 10 tests added

Copy link

changeset-bot bot commented Apr 22, 2025

⚠️ No Changeset found

Latest commit: f900b7c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

@tomasnyberg tomasnyberg left a comment

Choose a reason for hiding this comment

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

big improvement for lists and maps 🔥

discussed a bit offline to look at whether we can do without the reference check, will re-review

@@ -148,23 +158,34 @@ function validateFinalState(state: State) {
}
}

function applySpecialBreak(state: State, chunk: Chunk, nextChunk: Chunk) {
Copy link
Contributor

Choose a reason for hiding this comment

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

could this function clarify what all the booleans mean conceptually? I don't really understand why we have to check for no comments, and whether the next groups are going to break.

Also, not a huge fan of doing the overflowing calculation in here, I feel like it is risky to introduce that logic in more than one place (based on what we found previously)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add comments to explain them.
The overflowing calculation was redundant, now removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comment, re-review is required

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.

2 participants