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

For @types installing quickfix, only activate for implicit-any module #19394

Merged
1 commit merged into from
Oct 23, 2017

Conversation

ghost
Copy link

@ghost ghost commented Oct 20, 2017

Should fix the immediate issue in #19378. @mjbvz was getting an error since "lodash" wasn't resolving at all since he had classic module resolution set. Now we would not provide the codefix in that case (although we would still provide the refactoring).

@ghost ghost requested a review from mhegazy October 20, 2017 22:28
@mjbvz
Copy link
Contributor

mjbvz commented Oct 20, 2017

This is the opposite of my proposal on #19378. I believe install @types is a quick fix. It addresses a problem in your code even if there is no actually error currently. Refactoring to me suggests code motion

Instead, I propose we remove this action from refactorings and for now leave the quick fix implementation to only work when there is a diagnostic error in the code

@ghost
Copy link
Author

ghost commented Oct 20, 2017

@mhegazy Should we just disable the refactoring in release-2.6?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 21, 2017

This is the opposite of my proposal on #19378. I believe install @types is a quick fix. It addresses a problem in your code even if there is no actually error currently. Refactoring to me suggests code motion

I am not sure i understand this statement..

the way it is wired today, a quick fix is attached to a diagnostic message, and refactoring is one that is not attached to a diagnostic message.

our design should make it such that the quick fix is only shown if there an error, and the refactoring should be shown otherwise. I am not sure i understand the problem in #19378 to be able to comment on either proposed solutions.

@mjbvz
Copy link
Contributor

mjbvz commented Oct 21, 2017

I can understand this distinction on implementation side of things but I'm coming at the problem from a ux/client perspective.

Right now, VS Code presents both refactorings and quick fixes using the same lightbulb/code actions UI. However this may not always be the case. We have talked of creating a separate refactoring menu for example, and have also talked about changing the semantics of when refactorings are shown in the lightbulb menu.

I believe TypeScript's concept of refactorings and quick fixes should correspond with what a user would understand these to be in their editor: a refactoring is a code action that changes your code's structure while a quick fix is a code action that addresses some sort of problem with the code. Classifying install @types as a refactoring breaks this model. It is a quick fix because it addresses a problem in your code, and should grouped as such

@mhegazy
Copy link
Contributor

mhegazy commented Oct 23, 2017

let's get this into release-2.6 as well.

@ghost ghost merged commit f916e38 into master Oct 23, 2017
@ghost ghost deleted the codeFixCannotFindModule branch October 23, 2017 20:36
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants