Skip to content
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

Support completions that require changing from dot to bracket access #20547

Merged
10 commits merged into from Jan 9, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2017

Fixes #19433
Must wait on microsoft/vscode#39893 first.

This adds a completion for a property even if it can't follow a . -- and adds a code action to change the style from x.| to x["|"].

@ghost ghost force-pushed the completionsBracket branch 4 times, most recently from 27bec0d to 087493a Compare December 7, 2017 22:33
@ghost
Copy link
Author

ghost commented Dec 18, 2017

@mjbvz I just pushed a commit using replacementSpan and insertText (ref: #20730).

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 18, 2017

As I mentioned last week, there are people who've been using spaces in names to use as part of type system hacks to have pseudo-private members. Now something that starts with a space will bubble to the top.

@RyanCavanaugh had an idea of excluding names that are spaces, but we could generalize to any character that isn't a valid IdentifierStart

I think another idea might be is to de-prioritize names that aren't valid property accesses.

@@ -333,6 +333,7 @@ namespace ts {

export interface GetCompletionsAtPositionOptions {
includeExternalModuleExports: boolean;
includeBracketCompletions: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

If we ever want to support completing strings in object literals (without users needing to explicitly type in a quote character), does it make sense to call this something else?

Copy link
Author

Choose a reason for hiding this comment

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

It depends on whether we will still want a flag for the behavior of completing bracketed expressions, or if this will only be used in a non-user-facing way as an editor feature flag. I believe vscode had plans to make a user-facing options for includeExternalModuleExports in case some people don't want that.

Copy link
Contributor

@mjbvz mjbvz Dec 19, 2017

Choose a reason for hiding this comment

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

Yes we have an option for includeExternalModuleExports. However I think we would always want to enable includeBracketCompletions or whatever this new option is called. This flag is useful for backwards compatibility (see #20730) so that we don't break clients that don't support insertText

@ghost
Copy link
Author

ghost commented Dec 18, 2017

@DanielRosenwasser We could set sortText to a different value for anything that needs brackets, would that work?

@ghost ghost force-pushed the completionsBracket branch from 3a30279 to fd9ea5b Compare January 4, 2018 15:42
@mhegazy
Copy link
Contributor

mhegazy commented Jan 4, 2018

We could set sortText to a different value for anything that needs brackets, would that work?

I do not think this is a good idea. from a user perspective, there are searching for "foo bar" no reason why this should not next to "foo" just because when they commit it it becomes ["foo bar"] and not .foo.

We can filter out things that start with at least one sapace.. it is not common for property names to be written that way.

Copy link
Contributor

@mhegazy mhegazy left a comment

Choose a reason for hiding this comment

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

We need to filter out the ones that start with spaces as discussed earlier.

@ghost ghost force-pushed the completionsBracket branch from 761f2cb to 5091bfc Compare January 4, 2018 23:11
@ghost
Copy link
Author

ghost commented Jan 5, 2018

@mhegazy @DanielRosenwasser Good to go?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 9, 2018

//cc @chuckjaz

mjbvz added a commit to microsoft/vscode that referenced this pull request Jan 9, 2018
Requires a build of TS with microsoft/TypeScript#20547 Fixes #36429

Also relaxes the matching logic for suggestion items with filter texts
@ghost ghost merged commit 89ceb4b into master Jan 9, 2018
@ghost ghost deleted the completionsBracket branch January 9, 2018 02:57
@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants