Update rhtml files to comply with Herb#117
Update rhtml files to comply with Herb#117joelhawksley wants to merge 1 commit intoruby-gettext:masterfrom
Conversation
This pull request improves the parser by properly matching `<% end %>`
tags to `ERBCaseNode` when no `when`/`in` clauses are present.
For the following example:
```html+erb
<% case 1 %>
<% end %>
```
The reported errors are now reduced to 1, down from 3.
**Before**
```
Errors (3 total)
1. RubyParseError at test.html.erb:1:3
case_missing_conditions: expected a `when` or `in` clause after `case`
2. MissingERBEndTagError at test.html.erb:1:0
`<% case %>` started here but never closed with an end tag. The end tag may be in a different scope.
3. ERBControlFlowScopeError at test.html.erb:2:0
`<% end %>` appears outside its control flow block. Keep ERB control flow statements together within the same HTML scope (tag, attribute, or content).
```
**After**
```
Errors (1 total)
1. RubyParseError at test.html.erb:1:3
case_missing_conditions: expected a `when` or `in` clause after `case`
```
Inspired by ruby-gettext/gettext#117
/cc @joelhawksley
|
Nevermind! marcoroth/herb#1237 fixes this upstream. |
|
FWIW, the snippet in the pull request you touched is still invalid as this is invalid Ruby: case 1
endThe following is still going to raise an error in Herb: <% case 1 %>
<% end %>I just improved the way the parser handles this to only emit 1 instead of 3 errors. |
|
@marcoroth gotcha, thanks! |
|
Can we close this? |
|
@kou as @marcoroth said |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
OK.
Could you update the PR title and description to match the latest change? We'll use it for the commit message.
test/fixtures/erb/case.rhtml
Outdated
| <% case 1 %> | ||
| <% when 1 %> | ||
| <%# noop %> | ||
| <% end %> |
There was a problem hiding this comment.
Can we use the following?
<% case %>
<% when true %>
<% end %>I want to simplify this as much as possible.
| </div> | ||
| <p><%= _("Call a library method which has another textdomain.") %></p> | ||
| <div class="locale"><%= lib.hello %></div> | ||
| <div class="locale"><%= lib.hello %></div> |
There was a problem hiding this comment.
Do we need this for Herb? If it's not needed, could you revert it from this PR?
There was a problem hiding this comment.
It's not strictly needed, but both the Linter and Formatter correct it to remove the double space:
joelhawksley
changed the title
joelhawksley
force-pushed
the
herb-compat
branch
from
c4931a7 to
64c96b9
Compare
|
@kou I've updated the PR title and squashed the commits. |
kou
left a comment
kou
left a comment
There was a problem hiding this comment.
Could you rebase on master and update expected in tests?
| </div> | ||
| <p><%= _("Call a library method which has another textdomain.") %></p> | ||
| <div class="locale"><%= lib.hello %></div> | ||
| <div class="locale"><%= lib.hello %></div> |
|
BTW, how did you use Herb for our samples and tests? |
|
@kou the Herb linter runs via |
joelhawksley
force-pushed
the
herb-compat
branch
from
64c96b9 to
68576cd
Compare
|
@kou updated to latest master. |
|
CI is still failing. Could you enable GitHub Actions on your fork and check it? |
|
Can we install and run Herb linter with |
|
I believe the linter is only available as a JS package: https://herb-tools.dev/projects/linter |
joelhawksley
force-pushed
the
herb-compat
branch
from
68576cd to
5afd325
Compare
|
@kou I think this is in better shape again. Let me know whether you'd like me to add Herb to CI. |
The linter itself is only available via However, you can run |
3 participants
As part of working to integrate Herb, I ran into some issues when enabling Herb for all ERB and HTML in our application.
cc @marcoroth