Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Collection item scope provider #2683
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collection item scope provider #2683
Changes from all commits
7ad0871670afa83acc3a0d88f3709e83dada1c39093ff0843499f922e4e8d7b2896e132ba443445c045b3b959aeded5c797fb6dc678ebe5699aa3d00004e98a25d3294298d597630ea11ae5c32a390a19295e872d9c19372b92a3d956d707b56f75356172e3b4e5b9723ecc35ff8d1d4cc6092d90254ce794449ada66455e61b532951b947e3635ba033330837107fcd7031b678a4516File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as the other ones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTS: This is an example of the new code performing worse, but we understand why. This is because textual scope defines the item as:
In this case the language does identify that the comment is not part of the item, but we are preferring the textual scope, and as above, we no longer blind take the language server's view.
This will affect every language will you have an inline comment next to an item, we just don't have many tests in our more common languages that show that. There's an issue for improving the handling of comments around items. (Might be #1770?). It's a complex change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: the newer code uses more queries and predicates and less imperative TypeScript, so it's also something that would have been easier to monkey patch in the older code, although arguably, for a worse long term outcome
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTS: the old behavior was interesting: the cursor was on the comment so presumably the language server returned the entire :bongo item as the "item".
With the new behavior the textual scope includes the comment as part of the next item (see other thread), which is generally slightly worse, but in this case leads to a better outcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTS: this outcome is worse but there's not much we can do about it at the current time. Problem is that closure does not use explicit list delimiters and the textual base system can obviously not figure out if a space is part of an item or a separator here. The textual system thinks the entire
:foo "bar" :baz "whatever"is one "item".Andreas' idea: allow a language to specify whether it uses explicit delimiters for lists, and if it does not, go back to the old implementation of always taking the language servers opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#2723
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NTS: this is an expected change because of way we evaluate the textual and language sources with the change. Previously the language source would always be used if it was available; now we use both (
OneOf...) and compare the text ranges as a heuristic.In this situation the language server says
Some(1)is the item, the textual definition says1is the item, and we prefer that because of the nuances in this algorithm, which can frequently prefer the shorter range:cursorless/packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/compareTargetScopes.ts
Line 72 in 85fd827
(A related example, with the roles reversed, in Python;
import x, y, z, the language server will not include the import keyword; the original textual range would thinkimport xis the first item.). The comparison is applied to each item.This existing comparison logic is already used by all the other scope providers, such as surrounding pairs, so it's pretty well tested. There will be other cases where this changes though since legacy scopes were using a much simpler algorithm that iterates forward and returns the first containing item. This is not a design decision in this change, everything that uses the next generation scope providers uses this comparison function.
NB: Andreas says the Rust language definition possibly shouldn't even have a list item provider, because they're only necessary when the textual definition isn't sufficient (i.e., unusual delimiters); it's not clear why it is necessary but the language definition was provided by a contributor so we're not sure.
This file was deleted.
This file was deleted.