Fix text fragment extractor fallbacks #1863
Conversation
|
|
||
| const selectionWithEditor = { | ||
| editor, | ||
| selection: new Selection(range.start, range.end), |
There was a problem hiding this comment.
i think we have a range.toSelection. although looking at it, it has a boolean arg, ick. (maybe we could make that optional/named and default to false?)
There was a problem hiding this comment.
I think it's useful to force the user to think about the direction. If you don't care about a direction a selection is not that different from a range.
There was a problem hiding this comment.
sure. but at the call site .toSelection(false) is inscrutable.
...ess-vscode-e2e/src/suite/fixtures/recorded/surroundingPair/parseTree/python/changeString.yml
Show resolved
Hide resolved
| return surroundingRange; | ||
| } | ||
| // Search in the rest of the file, to find e.g. ("abc") | ||
| return findSurroundingPairTextBased( |
There was a problem hiding this comment.
Fwiw the specific bug that this PR is trying to fix was on this line: we were directly falling back to the text-based algorithm when the input range was not in a text fragment, when instead it should have fallen back to parse-tree-based
…ctors (#1862) - Depends on #1863 ## Checklist - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [/] I have updated the [docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and [cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet) - [/] I have not broken the cheatsheet - [x] merge #1858 first as seems required - [x] @pokey to fix a bug in the "processSurroundingPairCore" function atm that makes it not working atm (see commit message). It will be handled in a different commit before we can take this PR into account - [x] record a test for the fix by @pokey by saying "cursorless record" and using "change string" This PR also includes a few additional unit tests for Python strings. --------- Co-authored-by: Cedric Halbronn <[email protected]> Co-authored-by: Pokey Rule <[email protected]> Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
The fallback logic for determining whether to use text-based surrounding pairs or parse-tree-based was broken for text fragment extractors based on next-gen scope handlers (ie using the new
@textFragmenttag in a query file).This breakage meant that in a string like
"""hello"""in Python, the surrounding pair finder fell back to text-based, which resulted in it thinking"hello"was a string, rather than"""hello""", as would happen if it fell back to parse-tree-basedThis PR fixes the issue by unifying the fallback logic between legacy and next-gen text fragment extractors, so that they don't fall out of sync again
See also #1812 (comment); arguably that will make this PR irrelevant, but until then, it's better to have string not be broken when we migrate a language to use
@textFragmentNote that the tests in this PR don't actually test the code yet, because Python is still using legacy text fragment extractors. The tests will start biting when this PR is merged into main and then merged into #1862
Checklist