Skip to content

fix: move string lit check to trait#1074

Merged
fdncred merged 7 commits into
nushell:mainfrom
casedami:feat/abbreviations
May 16, 2026
Merged

fix: move string lit check to trait#1074
fdncred merged 7 commits into
nushell:mainfrom
casedami:feat/abbreviations

Conversation

@casedami
Copy link
Copy Markdown
Contributor

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

@fdncred fdncred requested a review from Copilot May 14, 2026 23:28
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 14, 2026

Thanks. Let's see what copilot thinks. @blindFS are you good with this solution?

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) -> bool method on the Highlighter trait returning false.
  • Moved the actual quote/escape-aware implementation onto ExampleHighlighter and removed it (and its rstest suite) from Editor.
  • 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.

Comment thread src/highlighter/mod.rs Outdated
Comment thread src/highlighter/example.rs
Comment thread src/highlighter/mod.rs Outdated
@blindFS
Copy link
Copy Markdown
Contributor

blindFS commented May 15, 2026

Thanks. Let's see what copilot thinks. @blindFS are you good with this solution?

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?

@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 15, 2026

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.

@casedami
Copy link
Copy Markdown
Contributor Author

casedami commented May 15, 2026

just appyling some of copilots comments about the docstrings, should otherwise be good to go

@fdncred fdncred merged commit e0ecf06 into nushell:main May 16, 2026
7 checks passed
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented May 16, 2026

Thanks

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.

4 participants