fix: move string lit check to trait#1074
Conversation
|
Thanks. Let's see what copilot thinks. @blindFS are you good with this solution? |
There was a problem hiding this comment.
Pull request overview
This PR refactors the is_inside_string_literal helper—originally introduced on Editor for the new fish-like abbreviations feature (#1060)—into a default-providing method on the Highlighter trait, with the concrete implementation moved to ExampleHighlighter. The two call sites in engine.rs (try_expand_abbreviation_at_cursor and parse_bang_command) are updated to dispatch through the highlighter, and the test helper is updated to install an ExampleHighlighter.
Changes:
- Added a default
is_inside_string_literal(&self, line, cursor) -> boolmethod on theHighlightertrait returningfalse. - Moved the actual quote/escape-aware implementation onto
ExampleHighlighterand removed it (and its rstest suite) fromEditor. - Updated abbreviation/bang call sites and a related test to use the highlighter; added one new test case for expansion after a closed string.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/highlighter/mod.rs | Adds is_inside_string_literal to the Highlighter trait with a false default. |
| src/highlighter/example.rs | Implements is_inside_string_literal on ExampleHighlighter. |
| src/core_editor/editor.rs | Removes the previous Editor::is_inside_string_literal and its parameterized tests. |
| src/engine.rs | Routes string-literal checks through self.highlighter; tweaks abbreviation tests and adds one new case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn is_inside_string_literal(&self, line: &str, cursor: usize) -> bool { | ||
| if line.is_empty() || cursor == 0 { | ||
| return false; | ||
| } | ||
| let mut in_single = false; | ||
| let mut in_double = false; | ||
| let mut escaped = false; | ||
| let mut byte_pos = 0; | ||
| for &byte in line.as_bytes() { | ||
| if byte_pos >= cursor { | ||
| break; | ||
| } | ||
| if escaped { | ||
| escaped = false; | ||
| byte_pos += 1; | ||
| continue; | ||
| } | ||
| match byte { | ||
| b'\\' => escaped = true, | ||
| b'\'' if !in_double => in_single = !in_single, | ||
| b'"' if !in_single => in_double = !in_double, | ||
| _ => {} | ||
| } | ||
| byte_pos += 1; | ||
| } | ||
| in_single || in_double | ||
| } |
There was a problem hiding this comment.
@casedami This wasn't just a doc comment. Help me understand if this was covered or not and why if not please.
There was a problem hiding this comment.
that's fair, i had removed it because i was thinking of incorporating similar tests on the nushell side since is_inside_string_literal would now be implemented on that side and also because i assumed the test cases wouldn't be necessary for the example highlighter struct. if you want i can add them to a test mod for that struct. alternatively, i can rework these test cases into engine.rs so there are such test cases for highlighters that override is_inside_string_literal as well as ones for the default implementation, which i think would be more useful. apologies for the oversight there on my part
There was a problem hiding this comment.
updated now, let me know if that looks good to you @fdncred
Yeah, LGTM. Although the actual challenge is on the nushell side, and I'm not even sure whether re-parsing is good enough if that's a low frequency op and refactoring current REPL implementation to support caching is not very straightforward, let's wait and see. BTW, I didn't know copilot can do it nowadays, is that service free? |
It's not free. It counts against my copilot license which is free for certain project maintainers. |
|
just appyling some of copilots comments about the docstrings, should otherwise be good to go |
applying changes from discussion in #1060
in order to test that expansion doesnt occur within a string literal i moved the original method to the example highlighter class as that seemed a good place for it, but if you think otherwise please let me know