Fix HAML extraction with embedded Ruby #17846
Conversation
| = succeed ',' do | ||
| = daisy_link("ViewComponent", "https://viewcomponent.org", target: "_blank") | ||
| = succeed ', and' do | ||
| = daisy_link "DaisyUI", "https://daisyui.com/", target: "_blank" | ||
| = daisy_link "TailwindCSS", "https://tailwindcss.com/", target: "_blank" |
There was a problem hiding this comment.
Note: the reason the = characters are back is because we are not replacing them in the HAML extractor itself anymore. Instead the value after the = is passed to the Ruby extractor.
| let actual = std::str::from_utf8(&processed).unwrap(); | ||
| let expected = include_str!("./test-fixtures/haml/dst-17051.haml"); | ||
|
|
||
| assert_eq!(actual, expected); |
There was a problem hiding this comment.
I find this a bit confusing to review. Looking at the snapshot alone I am really not sure if the right classes are extract. Should we run the extractor here and assert on the actual candidates being emitted?
There was a problem hiding this comment.
I don't feel like this snapshot is super helpful for reviewing code. I know that some stuff changed but it's really unclear which of the symbols are now considered candidates and which ones aren't. I'm fine with keeping the snapshot if we can somehow highlight candidates. High level it feels like these stuff should not be changed in a PR like this, expect if we have something that we under-classified or over-classified for some reason. WDYT?
There was a problem hiding this comment.
I agree with Philipp here. What if we added some kind of separator and then listed all the extracted candidates at the bottom of the fixture?
There was a problem hiding this comment.
Yep it's a bit hard to verify what's going on. We discussed this on Discord, but how about now?
Unfortunately for long lines GitHub doesn't always show it nicely, but now we're highlighting ^^^ under the extracted candidates
There was a problem hiding this comment.
This is much better — I've got one request tho can you move the commits and split things so there's the original annotated version before the fix and then the updated annotated version after the fix?
There was a problem hiding this comment.
Yeah can do that! I typically push a "failing test" first (which is the output of what we expect).
But let me split things here so the diff is clear between commits.
There was a problem hiding this comment.
@thecrypticace alright split things up, checkout the individual commits: https://github.com/tailwindlabs/tailwindcss/pull/17846/commits
Or the one commit that adjusts the annotations due to the fix: dd53aaa
RobinMalfait
force-pushed
the
fix/issue-17813
branch
from
a941633 to
83f3926
Compare
|
Thank you @RobinMalfait really appreciate it |
This PR fixes a Windows CI issue due to the recently merged #17846 There might be better ways to solve this, but essentially we want to make sure we are always dealing with `\n` and not `\r\n`. This PR fixes that by replacing all `\r\n` with `\n` in the tests.
This PR fixes and improves the HAML extractor by ensuring that whenever we detect lines or blocks of Ruby code (
-,=and~) characters at the beginning of the line, we treat them as Ruby code.Fixes: #17813
Test Plan
Running this in the internal extractor tool you can see that the
w-[12px],w-[16px],h-[12px], andh-[16px]are properly extracted.Note: the
mr-12pxis also extracted, but not highlighted because this is not a valid Tailwind CSS class.