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

Use syntax-tree-erb #25

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Use syntax-tree-erb #25

wants to merge 1 commit into from

Conversation

elia
Copy link
Member

@elia elia commented Jul 7, 2023

Consider joining forces with https://github.com/davidwessman/syntax_tree-erb as the project has done an awesome job already and with a few fixes we could get feature parity.

Currently trying to run it on existing fixtures has the following issues:

  • "test/fixtures/if_then_else.html.erb" → Found no matching erb-tag to the if-tag at line 6, char 208..351
  • Comment ERB tags are rendered with a space before the # char, might be intentional tho
  • Whitespace is not preserved between HTML tags: adding newlines around Foo in <span>Foo</span> can disrupt how the HTML is rendered
  • Whitespace is not preserved between ERB tags: adding newlines inside %><%= in <%= "Foo" %><%= "Bar" %> can disrupt how the HTML is rendered

cc @davidwessman the pretty print based approach of SyntaxTree is far superior to the long formatting script in this project, I thought was good to let you know I'm considering switching to it entirely in case you're interested in helping out 🙌

@elia elia self-assigned this Jul 7, 2023
@davidwessman
Copy link

Cool 😊 I am totally up for helping out, will check it out when I'm back at my computer.

@davidwessman
Copy link

I have been off for the summer, but I am now looking at how to handle whitespace differntly.
Not satisfied with any solution yet :/

@davidwessman
Copy link

@elia
Could you test out davidwessman/syntax_tree-erb#51 and see if it makes sense to you?

This was my third take and the first time I managed to make all test pass.
It feels like this will give more edge-cases, maybe I will have to reconsider it a few more times.

@davidwessman
Copy link

davidwessman commented Sep 3, 2023

@elia
Now I feel more pleased with the changes syntax_tree-erb does on your test cases.
Please give v0.10.5 a try 🙂 I do not have any good idea for handling e.g. frontmatter yet.

@elia
Copy link
Member Author

elia commented Sep 3, 2023

@davidwessman that's awesome! I just checked it out and it works perfectly 🙌
I think for now we can have a thin layer removing and re-adding the front matter whenever is detected like it's currently done in erb-formatter.

I'll give it a crack and send a PR if I get somewhere 👍

@davidwessman
Copy link

Nice 😊

I am thinking about how to handle style and javascript-blocks, so I might be able to solve frontmatter as well.

Is ERB allowed inside the frontmatter?

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