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

Add custom Jayvee formatter #562

Merged
merged 9 commits into from
May 16, 2024
Merged

Add custom Jayvee formatter #562

merged 9 commits into from
May 16, 2024

Conversation

georg-schwarz
Copy link
Member

@georg-schwarz georg-schwarz commented May 13, 2024

Adds a custom formatter to via the language server protocol.
Future updates might enable using the formatted from CLI.

Works around an issue with comments not being correctly formatted: eclipse-langium/langium#1351

@georg-schwarz georg-schwarz requested a review from joluj May 14, 2024 07:57
@georg-schwarz georg-schwarz marked this pull request as ready for review May 14, 2024 08:26
Copy link
Contributor

@joluj joluj left a comment

Choose a reason for hiding this comment

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

These points can be tackled in other PRs though. But we should open issues at least.

example/electric-vehicles.jv Outdated Show resolved Hide resolved
example/workbooks-xlsx.jv Outdated Show resolved Hide resolved
@georg-schwarz
Copy link
Member Author

Thx, good catches!

I noticed in the meanwhile that Langium itself does AST-node-specific formatting. Probably sth we should consider as well (e.g., for the colons).
https://github.com/eclipse-langium/langium/blob/main/packages/langium/src/grammar/lsp/grammar-formatter.ts

@joluj
Copy link
Contributor

joluj commented May 16, 2024

I noticed in the meanwhile that Langium itself does AST-node-specific formatting. Probably sth we should consider as well (e.g., for the colons).

I've tried that when I've created the initial POC. For the colons end brackets, I decided against it. With it, we need to explicitly provide every relevant AST element, which seemed laborious for a POC. However, since it goes to production now, we should reevaluate.

@georg-schwarz
Copy link
Member Author

georg-schwarz commented May 16, 2024

Fixed the issues and converted all examples to use 2 spaces.

Every file should end with exactly one empty line https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline

That should be tackled in a separate issue (#567).

In general, I'm not too happy with the implementation. At some points two formatting are valid that the formatting function switches between when applied. But I'll leave that to future PRs, together with the refactoring to apply formatting not globally but AST node specifically.

@georg-schwarz georg-schwarz merged commit 8c770d5 into main May 16, 2024
3 checks passed
@georg-schwarz georg-schwarz deleted the jayvee-formatter branch May 16, 2024 16:46
@github-actions github-actions bot locked and limited conversation to collaborators May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants