Repository navigation
Fix class extraction for Rails' strict locals - #19525
Conversation
This allows for strict-locals support.
This allows us to verify what the expected extracted candidates are. We already have `test_extract_contains`, which just ensures that the provided list is extracted (while ignoring some potential noise).
We introduced skipping comments in the Ruby extractor for issue #19239 This adds a regression test to make sure that the behavior is unchanged
RobinMalfait
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/oxide/src/extractor/pre_processors/pre_processor.rs (2)
28-51: Exact extraction helper is correct; consider robustness to orderingThe helper correctly reuses
Extractorand gathers both candidates and CSS variables, and the comparison logic is sound. If extraction order is not semantically important, you might want to normalize (e.g., sort) bothcandidatesandexpectedbefore comparing to avoid brittle tests tied to traversal order.
54-75: Containment helper rename and loop update look goodRenaming
items→expectedand iteratingfor item in &expectedkeeps semantics intact and improves readability; the containment check againstcandidatesremains correct.You could consider sharing the candidate-collection logic between
test_extract_exactandtest_extract_containsto avoid duplication, but this is purely a test ergonomics tweak.crates/oxide/src/extractor/pre_processors/ruby.rs (1)
403-429:test_strict_localsaccurately captures the Rails strict‑locals scenarioThe test input mirrors the
<%# locals: (css: "…") %>pattern plus additional locals and static classes, andtest_extract_containsasserts that all relevant class names (text-amber-600,text-sky-500,text-green-500) are discoverable. This directly validates the new guard around#comment stripping and provides solid coverage for the original bug report.If you later want stronger guarantees here, you could switch to
test_extract_exactwith a full expected list to ensure no unexpected candidates are introduced from the strict‑locals line.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.mdcrates/oxide/src/extractor/pre_processors/pre_processor.rscrates/oxide/src/extractor/pre_processors/ruby.rs
🧰 Additional context used
🧬 Code graph analysis (2)
crates/oxide/src/extractor/pre_processors/ruby.rs (1)
crates/oxide/src/extractor/pre_processors/pre_processor.rs (3)
test(5-25)test_extract_exact(28-51)test_extract_contains(54-85)
crates/oxide/src/extractor/pre_processors/pre_processor.rs (3)
crates/oxide/src/extractor/mod.rs (1)
new(65-72)crates/oxide/src/cursor.rs (1)
new(30-42)crates/oxide/src/extractor/machine.rs (1)
new(13-15)
🔇 Additional comments (3)
CHANGELOG.md (1)
16-16: Changelog entry is consistent and clearThe new “Rails' strict locals” fix entry matches surrounding formatting and scope; no changes needed.
crates/oxide/src/extractor/pre_processors/ruby.rs (2)
122-126: Guarding comment stripping by previous byte matches strict‑locals requirementConditioning the
#comment handling oncursor.prevnot being%ensures<%# locals: … %>blocks are no longer treated as comments while ordinary Ruby#comments (including the RDoc cross‑reference examples and trailing comments after code) are still stripped. This aligns with the strict‑locals use case without impacting%w#…#handling or string literals.
390-401:test_skip_commentsgives a precise regression check for comment handlingThis test nicely locks in the expectation that pure comment blocks (including RDoc cross‑refs) yield no extracted candidates by using
test_extract_exactwith an empty vector. That’s a good targeted regression for the earlier “skip comments” behavior.
Fixes: #19481
This PR improves the Ruby extractor to better handle strict locals. We recently introduced skipping comments in the Ruby extractor (PR #19243 for #19239) by ignoring comments that start with
#until the end of the line.Strict locals are implemented like this:
Notice the
#after the<%, we considered this a comment and ignored it.This PR changes that behavior slightly where we skip comments that are preceded by
%. This means that<%# anything here _will_ be scanned %>. This should solve the strict locals case, and normal comments will still be skipped.We can be more strict in the future if needed, but I think that this should be a good solution for both scenarios.
Test plan
We can also verify the extracted candidates:
(it's subtle, but you can see that the class is being extracted now)