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

LanguageService - getSuggestionDiagnostics, getEditsForRefactor, getCodeFixesAtPosition #488

Merged
merged 19 commits into from
Nov 8, 2018

Conversation

cancerberoSgx
Copy link
Contributor

The first commit implements LanguageService getEditsForRefactor and getCodeFixesAtPosition and wraps all the data types used by those.

The last commit shows how this can be useful to (re)use exsting TypeScript refactors in ts-simple-ast. In this case I implemented ArrowFunction.addBracesToBody and ArrowFunction.removeBracesToBody just using existing typescript code fix "Add or remove braces in an arrow function". And also implemented Statement.moveToNewFile, using existing typescript refactor "Move to a new file".

I think TypeScript team slowly starts to focus on providing useful refactoring tools and I think ts-simple-ast should use these to enrich its high level APIs. I can keep adding more refactors provided by TS but first I wanted to ask you what do you think.

@coveralls
Copy link

coveralls commented Oct 31, 2018

Coverage Status

Coverage increased (+0.2%) to 94.99% when pulling 7654477 on cancerberoSgx:moveToNewFile into c026a16 on dsherret:dev.

Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

The LanguageService changes look good, but I don't want to have the refactors that cause the nodes to be forgotten to be so easily accessible.

src/compiler/ast/function/ArrowFunction.ts Outdated Show resolved Hide resolved
src/compiler/ast/statement/Statement.ts Outdated Show resolved Hide resolved
src/compiler/tools/LanguageService.ts Outdated Show resolved Hide resolved
src/compiler/tools/results/TextChange.ts Outdated Show resolved Hide resolved
@dsherret
Copy link
Owner

@cancerberoSgx The LanguageService changes look good, but I'm sorry I keep denying code. Could you please read the contributing instructions for submitting features? That would help us not waste effort. Thanks! https://github.com/dsherret/ts-simple-ast/blob/master/CONTRIBUTING.md#contributing-features

@cancerberoSgx
Copy link
Contributor Author

cancerberoSgx commented Nov 1, 2018

@cancerberoSgx The LanguageService changes look good, but I'm sorry I keep denying code. Could you please read the contributing instructions for submitting features? That would help us not waste effort. Thanks! https://github.com/dsherret/ts-simple-ast/blob/master/CONTRIBUTING.md#contributing-features

No worries! Will do it tomorrow, that's why I separate it in two different commits. I insist on having this discussions on a PR instead of on an issue since it's much more clear and is not a big deal for me, but in the future I will create an issue first.

I understand. forgotten why these cannot" ? I think I want to have the discussion at least. There is another code fix that is as useful as Organize Imports - the one that will remove all declarations or symbols that are not used. So I will keep this functions in a separate library of mine. Do you think I could still contribute it to ts-simple-ast as a separate class/utility at least?

@cancerberoSgx
Copy link
Contributor Author

I think is ready now, thanks

@dsherret
Copy link
Owner

dsherret commented Nov 3, 2018

@cancerberoSgx I'm going to quickly make all the changes and merge this PR. Will have it done in 15-30 mins so don't bother with any changes at the moment.

@dsherret
Copy link
Owner

dsherret commented Nov 3, 2018

Hey @cancerberoSgx, this is almost done. I made some updates because it was quicker than reviewing. We just need a few tests for the remaining comments I made before this is merged. I've stopped working on this now so if you have the time to add those tests then please add them. Otherwise, I will get to it sometime next weekend.

@cancerberoSgx
Copy link
Contributor Author

added more test asserts

Copy link
Owner

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

We'll need to fix the CI errors (issue with declaration file... fix outlined in my comments)

import { DefinitionInfo, EmitOutput, FileTextChanges, ImplementationLocation, RenameLocation, TextChange } from "./results";
import { DefinitionInfo, EmitOutput, FileTextChanges, ImplementationLocation, RenameLocation, TextChange, DiagnosticWithLocation } from "./results";
import { RefactorEditInfo } from "./results/RefactorEditInfo";
import { CodeFixAction } from "./results/CodeFixAction";
Copy link
Owner

Choose a reason for hiding this comment

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

These two imports aren't included in the barrel file (./results/index.ts). Could you add them there and then fix these imports?

That should fix these CI errors:

> ts-node --transpileOnly scripts/verification/ensureNoDeclarationFileErrors
dist-declarations/ts-simple-ast.d.ts:8897:214 - error TS2304: Cannot find name 'RefactorEditInfo'.
8897     getEditsForRefactor(filePathOrSourceFile: string | SourceFile, formatOptions: FormatCodeSettings, positionOrRange: number | TextRange, refactorName: string, actionName: string, preferences?: UserPreferences): RefactorEditInfo | undefined;
                                                                                                                                                                                                                          ~~~~~~~~~~~~~~~~
dist-declarations/ts-simple-ast.d.ts:8907:202 - error TS2304: Cannot find name 'CodeFixAction'.
8907     getCodeFixesAtPosition(filePathOrSourceFile: string | SourceFile, start: number, end: number, errorCodes: ReadonlyArray<number>, formatOptions?: FormatCodeSettings, preferences?: UserPreferences): CodeFixAction[];
                                                                                                                                                                                                              ~~~~~~~~~~~~~
There were declaration file issues!

@dsherret dsherret changed the base branch from master to dev November 6, 2018 16:59
@dsherret dsherret changed the title languageService & refactors LanguageService - getSuggestionDiagnostics, getEditsForRefactor, getCodeFixesAtPosition Nov 8, 2018
@dsherret dsherret merged commit 9e42b10 into dsherret:dev Nov 8, 2018
@dsherret
Copy link
Owner

dsherret commented Nov 8, 2018

Thanks once again @cancerberoSgx! :)

dsherret pushed a commit that referenced this pull request May 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants