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

Why do I get lightbulbs/refactorings on all quoted strings (among other things)? #33972

Closed
chrisdias opened this issue Sep 7, 2017 · 3 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug javascript JavaScript support issues typescript Typescript support issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded

Comments

@chrisdias
Copy link
Member

I seem to get a lot of lightbulbs. For examaple, when I highlight the entire line here why am i getting a refactoring?

image

If i put the cursor in the 'docker' string I get a lightbulb:

image

I move the cursor to getConfiguration and the lightbulb goes away...

image

Actually, it looks like no matter where I have a quoted string, I get a lightbulb. Here my cursor is in "Images":

image

And when I tell it to refactor my code into a new function, I get this which is technically correct I guess, but not very useful as far as I can tell.

    private newFunction(): string {
        return "Images";
    }
@chrisdias chrisdias changed the title Why do I get lightbulbs/refactorings on all quoted strings? Why do I get lightbulbs/refactorings on all quoted strings (among other things)? Sep 7, 2017
@mjbvz mjbvz added javascript JavaScript support issues typescript Typescript support issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) labels Sep 8, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 8, 2017

This looks like a bug on our side for strings. The lightbulb should only show up when you either have a selection or when the cursor is in a range with a diagnostic of some kind

More broadly, we should really not support extracting literals like this at all. This is tracked upstream by microsoft/TypeScript#18060

@mjbvz mjbvz added the insiders label Sep 8, 2017
@mjbvz
Copy link
Collaborator

mjbvz commented Sep 8, 2017

Looks like insiders only. 1.16 not effected in my testing

@jrieken Was the intent to only keep in the #33241 empty selection fix for 1.16 and then revert it in master? (d8ec15e) If so, I'll push harder to get the TS fix in for TS 2.5.3 and see if we can inline the empty selection logic in the TS extension side for now

@mjbvz mjbvz added the bug Issue identified by VS Code Team member as probable bug label Sep 8, 2017
@jrieken
Copy link
Member

jrieken commented Sep 8, 2017

Was the intent to only keep in the #33241 empty selection fix for 1.16 and then revert it in master? (d8ec15e) I

Yes. Having lightbulbs on empty selections is valid, think of a invert-if-else hint at the end of an if-statement, more here: #33241 (comment)

On the good side, the lightbulb oracle now uses the range of the selection (unless it's a diagnostics) and the provider can check how much is selected. E.g. I think it useful to offer 'extract method' when having a single statement selected, it should be at least 2 statements.

@mjbvz mjbvz added this to the September 2017 milestone Sep 14, 2017
@mjbvz mjbvz closed this as completed in 6d6d83b Sep 19, 2017
@roblourens roblourens added the verified Verification succeeded label Sep 28, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug javascript JavaScript support issues typescript Typescript support issues upstream Issue identified as 'upstream' component related (exists outside of VS Code) verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants