GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Allow quoted names in completions · Pull Request #18162 · microsoft/TypeScript

Conversation

@ghost

@ghost Deleted user (ghost) commented Aug 30, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Fixes #17040
This was a pretty simple change as I just had to change the code that had previously been returning undefined on any quoted name.
There may be objections to this as there were to #16864, so CC Ron Buckton (@rbuckton) Wesley Wigham (@weswigham)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson
Allow quoted names in completions
ad1a5e8
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Aug 30, 2017
weswigham
Wesley Wigham (weswigham) requested changes Aug 30, 2017
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

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.

I don't actually have any objections to this, conceptually, since we've said that we wanted to offer completions for strings of all kinds for awhile. I do have a remark on the regex being used for number matching, though.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/services/completions.ts Outdated
}

function isNumericLiteralText(name: string): boolean {
return /^-?\d+(\.\d+)?(e\d+)?$/.test(name);

@weswigham Wesley Wigham (weswigham) Aug 30, 2017

Copy link
Copy Markdown
Member

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.

There are contexts like property names:

            const foob = {
                -0() {
                    return 1;
                }
            };

where a leading minus would never be allowed (only an identifier, string literal, or number) - I don't think this should include the check for -??

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Aug 30, 2017

Copy link
Copy Markdown
Member

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.

Also, this helper probably should be somewhere central, since it will need to be updated once we support the numeric seperator proposal.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ghost Deleted user (ghost) Aug 30, 2017 •
edited by ghost
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Author

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.

We can just use a string literal in this case then, which makes this function unnecessary. 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Andy Hanson
Don't allow string literal completions if not in an object literal; a…
f28783e 
…nd use string literals for number keys
mhegazy
Mohamed Hegazy (mhegazy) reviewed Sep 6, 2017
View reviewed changes
Comment thread src/services/completions.ts
// We, thus, need to check if whatever was inside the quotes is actually a valid identifier name.
if (performCharacterChecks && !isIdentifierText(name, target)) {
return undefined;
return allowStringLiteral ? JSON.stringify(name) : undefined;

@mhegazy Mohamed Hegazy (mhegazy) Sep 6, 2017

Copy link
Copy Markdown
Contributor

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.

that would be another place where a user-defined quote setting will need to be honored.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
mhegazy
Mohamed Hegazy (mhegazy) approved these changes Sep 6, 2017
View reviewed changes
Andy Hanson
Add TODO
572cea6
@ghost
ghost merged commit 36607e1 into master Sep 6, 2017
@ghost
ghost deleted the completions_quoted branch September 6, 2017 21:39
@microsoft Microsoft (microsoft) locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@weswigham @mhegazy @msftclas
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.