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

Fix formatter #1628

Merged
merged 7 commits into from
Aug 14, 2024
Merged

Fix formatter #1628

merged 7 commits into from
Aug 14, 2024

Conversation

ydaveluy
Copy link
Contributor

Hello,

This PR addresses several issues in the current formatter implementation:

  • Return only the necessary edits: This ensures that only the edits which actually change the document are returned, rather than all possible edits.

  • Fix avoidOverlappingEdits: This change improves the removal of overlapping edits, ensuring they are handled correctly across multiple levels.

  • Prevent formatting on the same line: This update prevents formatting actions from being applied to the same line if the previous node is hidden.

These improvements aim to enhance the accuracy and efficiency of the formatter. Please let me know if you have any questions or need further clarifications.

@cdietrich
Copy link
Contributor

cdietrich commented Aug 14, 2024

Can we double check if this won’t reintroduce https://github.com/eclipse-langium/langium/pull/1550/files #1469

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. Just one issue (on Windows systems, see below). Can you also please create a test case for that: You will need to manually invoke the Formatter service for that, and assert that no text edits are returned for a correctly formatted document (ideally with formattings that are doing newline or indentation).

packages/langium/src/lsp/formatter.ts Outdated Show resolved Hide resolved
@msujew
Copy link
Member

msujew commented Aug 14, 2024

@cdietrich I can't imagine this reintroducing #1362, but I also don't have a test case for this anymore. Do you still have one lying around and try it against this branch?

@cdietrich
Copy link
Contributor

@msujew unfortunately i have no langium dev env available as linking will mess up everything.
do you have an idea how to try it less invasively?

@msujew
Copy link
Member

msujew commented Aug 14, 2024

@cdietrich You could add the test repo as a temporary part the examples of this branch. That way, npm should do the linking for you.

@ydaveluy There is a linting error, see https://github.com/eclipse-langium/langium/actions/runs/10387365742/job/28760687114?pr=1628

@cdietrich
Copy link
Contributor

@msujew thx. looks good. formatter stays idempotent

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

@cdietrich Thanks for checking!

@msujew msujew added the LSP Language Server Protocol integration label Aug 14, 2024
@msujew msujew added this to the v3.2.0 milestone Aug 14, 2024
@msujew msujew merged commit 6690076 into eclipse-langium:main Aug 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
LSP Language Server Protocol integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants