- Notifications
You must be signed in to change notification settings - Fork13.2k
Introduce an organizeImports command#21909
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
amcasey commentedFeb 12, 2018
This will need to be reconciled with#21876. |
src/services/services.ts Outdated
| } | ||
| } | ||
| constsortedImportSpecifiers=stableSort(newImportSpecifiers,(s1,s2)=>{ |
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.
This comparison should be shared with#21876.
src/services/services.ts Outdated
| if(isImportDeclaration(node)){ | ||
| oldImportDecls.push(node); | ||
| } | ||
| // TODO (https://github.com/Microsoft/TypeScript/issues/10020): sort *within* ambient modules (find using isAmbientModule) |
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.
Oops, forgot to mention this other TODO in the description.
amcasey commentedFeb 13, 2018
I don't understand the build failures. I ran |
amcasey commentedFeb 13, 2018
|
weswigham commentedFeb 13, 2018
I think our mistake is that windows shells don't expand globbing patterns natively? Probably? I'm not sure. |
| } | ||
| } | ||
| functionassertListEqual(list1:ReadonlyArray<Node>,list2:ReadonlyArray<Node>){ |
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 think it would be simpler to emit and compare strings, vs parsing and comparing ASTs. That way there's no need to writeassertEqual and we also test if we are correctly formatting the result. Also would make error messages more readable.
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.
Suits me. How do I stringify them?
ghostFeb 13, 2018 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
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'm attempting to do this, but the emitter works best if the ast it's working on is eitherentirely synthesized, or not at all. I'm debugging and it looks like there are some nodes withpos set as children of a synthesized node, which causes the emitter to look at the source file for comments, only there is no source text. Some part of organizeImports might be forgetting to clone a node?
On this subject, we should have fourslash tests of this feature to ensure we're testing the whole thing, including emit.
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.
Regarding testing with text, it seems not to have been simpler. I'm going to leave things as they are until we have a concrete problem to correct or improvement to make.
I was under the impression that the baseline tests were doing an end-to-end test. Did you mean something else?
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.
No, baseline tests will do.
| returncodefix.getAllFixes({ fixId, sourceFile, program, host, cancellationToken, formatContext}); | ||
| } | ||
| functionorganizeImports(scope:OrganizeImportsScope,formatOptions:FormatCodeSettings):ReadonlyArray<FileTextChanges>{ |
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 think these changes are substantial enough to deserve anorganizeImports.ts file, just like we have for breakpoints/classifier/etc.
| assertListEqual(expectedCoalescedImports,actualCoalescedImports); | ||
| }); | ||
| it("Combine namespace imports",()=>{ |
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.
Name is "Combine namespace imports" but we're actually testing that wedon't combine them
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.
The name describes the action taken, rather than the outcome. Combining namespace imports happens to be a no-op.
| }); | ||
| it("Combine namespace import with default import",()=>{ | ||
| constsortedImports=parseImports( |
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.
This pattern occurs a lot, could there be a helper so this can be written:
assertCoalesce([`import * as x from "lib";`,`import y from "lib";`],`import y, * as x from "lib";`)
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 left it expanded because I thought it made the test more readable. A list of unnamed strings can be hard to follow.
| assert.equal(testPath,changes[0].fileName); | ||
| Harness.Baseline.runBaseline(baselinePath,()=>{ | ||
| constdata:string[]=[]; |
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.
Since the things we push todata are the same, this could just be an array literal["// ==ORIGINAL==", testContent, "// ==ORGANIZED==", textChanges.applyChanges(testContent, changes[0].textChanges)].join(newLineCharacter).
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.
Frankly, I didn't understand why we didn't just build a string directly. Is the array literal preferable?
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.
Well, it lets you avoid writing${newLineCharacter} a lot... though that's hardcoded to\n anyway, so you could just write\n.
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.
In older JS runtimes before everyone switched to ropes, repeated string concat was expensive.string[] -> onestring viajoin is basically the equivalent of using aStringBuilder in C#
src/services/organizeImports.ts Outdated
| cancellationToken:CancellationToken){ | ||
| // All of the (old) ImportDeclarations in the file, in syntactic order. | ||
| constoldImportDecls:ImportDeclaration[]=[]; |
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.
const oldImportDecls: ReadonlyArray<ImportDeclaration> = sourceFile.statements.filter(isImportDeclaration); -- should be fast enough that you don't need to check cancellationToken inside the filter.
Actually, I would just removecancellationToken unless we actually see a perf problem...
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.
You could also filter out invalid imports here, and be able to assume thatgetExternalModuleName returns a defined result everywhere else.
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.
Fair. It was mostly there so I could test the wait dialog behavior.
src/services/organizeImports.ts Outdated
| cancellationToken.throwIfCancellationRequested(); | ||
| // All of the (new) ImportDeclarations in the file, in sorted order. | ||
| constnewImportDecls=coalescedImportDecls; |
ghostFeb 14, 2018 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
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.
Just namecoalescedImportDeclsnewImportDecls?
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.
Are locals expensive in TS? I like naming things - it makes code easier to read.
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'm sure modern JS engines know that these are the same thing. I normally agree that giving a descriptive name for a variable is useful, butconst coalescedImportDecls = coalesceImports(sortedImportDecls); just repeats the same information in two different names, since the function name already tells you what it's doing -- same for the above lines.
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.
In the debugger, it's easy to inspect each stage individually. If you'd prefer not to have any redundant text, arguably they should be combined into a single expression with only final value stored in a named variable.
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.
For pure functions, you can enter e.g.coalesceImports(sortedImportDecls) into the watch list for inspection without needing a named local variable.
src/services/organizeImports.ts Outdated
| } | ||
| else{ | ||
| // Delete the surrounding trivia because it will have been retained in newImportDecls. | ||
| constreplaceOptions={ |
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.
InlinereplaceOptions, that way we get a contextual type and can warn on misnamed properties.
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.
Sure.
src/services/organizeImports.ts Outdated
| changeTracker.replaceNodeWithNodes(sourceFile,oldImportDecls[0],newImportDecls,replaceOptions); | ||
| } | ||
| constchanges=changeTracker.getChanges(); |
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.
return changeTracker.getChanges(); (remember the debugger will let you see the return value even if it is not named.)
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.
Sure
| } | ||
| functiongetExternalModuleName(specifier:Expression){ | ||
| returnisStringLiteral(specifier)||isNoSubstitutionTemplateLiteral(specifier) |
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.
#21953 would be useful here.
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.
Thanks!
src/services/organizeImports.ts Outdated
| /*@internal */// Internal for testing | ||
| exportfunctionsortImports(oldImports:ReadonlyArray<ImportDeclaration>){ | ||
| if(oldImports.length<2){ |
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.
- In
core.ts:
exportfunctioncompareBooleans(a:boolean,b:boolean):Comparison{returncompareValues(a ?1 :0,b ?1 :0);}
- Perform
length < 2optimization insidestableSort - Here:
returnstableSort(oldImports,({moduleSpecifier:m1},{moduleSpecifier:m2})=>{constname1=getExternalModuleName(m1)||m1.getText();constname2=getExternalModuleName(m2)||m2.getText();returncompareBooleans(isExternalModuleNameRelative(name1),isExternalModuleNameRelative(name2))||compareStringsCaseSensitive(name1,name2);});
This would break thenon-relative vs invalid andrelative vs invalid tests, but I don't think it really matters how we sort invalid expressions in import positions.
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'm not sure I understand your concerns about the current implementation.
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.
It's not that I spotted any bugs, just noticed that it could be done in 4 lines.
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.
It hadn't occurred to me to combineComparisons with|| - applying that should clean things up a bit.
One of the reasons I pulled out the struct was so that the same work wouldn't be repeated for every comparison in which a given import participated. If that's not interesting, it can obviously be simplified.
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.
If you filter out invalid imports at the top,getExternalModuleName is just a property lookup.isExternalModuleNameRelative isn't very complicated either -- some simple regex andindexOf tests.
src/services/organizeImports.ts Outdated
| return[]; | ||
| } | ||
| constusedImportDecls=removeUnusedImports(oldImportDecls); |
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.
Probably too many calls tothrowIfCancellationRequested here - in general you only need to call this every ~50-200ms, and I don't thinksortImports orcoalesceImports could ever be that slow.
| *@param sortedImports a list of ImportDeclarations, sorted by module name. | ||
| */ | ||
| exportfunctioncoalesceImports(sortedImports:ReadonlyArray<ImportDeclaration>){ | ||
| if(sortedImports.length===0){ |
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.
You already tested for length at the top oforganizeImports, and sorting wouldn't have changed the length.
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 believe this also runs after unused import removal.
| *@param sortedImports a non-empty list of ImportDeclarations, sorted by module name. | ||
| */ | ||
| functiongroupSortedImports(sortedImports:ReadonlyArray<ImportDeclaration>):ReadonlyArray<ReadonlyArray<ImportDeclaration>>{ | ||
| Debug.assert(length(sortedImports)>0); |
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.
functiongroupSortedImports(sortedImports:ReadonlyArray<ImportDeclaration>):ReadonlyArray<ReadonlyArray<ImportDeclaration>>{returngroupBy(sortedImports,i=>getExternalModuleName(i.moduleSpecifier));}functiongroupBy<T>(values:ReadonlyArray<T>,by:(value:T)=>string):ReadonlyArray<ReadonlyArray<T>>{Debug.assert(values.length!==0);constgroups:T[][]=[];letgroupName=by(values[0]);letgroup:T[]=[];for(constvalueofvalues){constb=by(value);if(b===groupName){group.push(value);}else{if(group.length){groups.push(group);}groupName=b;group=[value];}}if(group.length){groups.push(group);}returngroups;}
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.
Maybe better:
functiongroupSortedImports(sortedImports:ReadonlyArray<ImportDeclaration>):ReadonlyArray<ReadonlyArray<ImportDeclaration>>{returngroupBy(sortedImports,(a,b)=>getExternalModuleName(a.moduleSpecifier)===getExternalModuleName(b.moduleSpecifier));}functiongroupBy<T>(values:ReadonlyArray<T>,areSameGroup:(a:T,b:T)=>boolean):ReadonlyArray<ReadonlyArray<T>>{Debug.assert(values.length!==0);constgroups:T[][]=[];letgroup:T[]=[];for(constvalueofvalues){if(group.length&&!areSameGroup(value,group[0])){groups.push(group);group=[];}group.push(value);}if(group.length){groups.push(group);}returngroups;}
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.
Just to confirm, your improvement was pulling out a generic helper? The actual behavior is the same?
ghostFeb 14, 2018 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
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.
Yes, assuming invalid imports are filtered out (sogetExternalModuleName returns a defined result).
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.
Just noticed we don't needDebug.assert(values.length !== 0); in the "maybe better" version.
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.
Note that this isn't true group-by - it requires that the input be sorted. Is it still worth pulling out a generic helper for specialized functionality?
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.
Actually, this doesn'tjust rely on it being sorted, but on it being sorted by the same mechanism asareSameGroup. Maybe it would be better to group first (not relying on sorting) and then sort the groups?
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.
Yes, that's why it's a private helper function. At the moment, there doesn't seem to be a need for something more general, does there?
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.
As you requested, we are now grouping before performing the other operations.
src/services/organizeImports.ts Outdated
| cancellationToken:CancellationToken){ | ||
| // All of the (old) ImportDeclarations in the file, in syntactic order. | ||
| constoldImportDecls:ImportDeclaration[]=[]; |
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.
You could also filter out invalid imports here, and be able to assume thatgetExternalModuleName returns a defined result everywhere else.
src/services/organizeImports.ts Outdated
| returncoalescedImports; | ||
| // `undefined` is the min value. | ||
| functioncompareIdentifiers(s1:Identifier|undefined,s2:Identifier|undefined){ |
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.
return compareStringsCaseSensitive(s1 && s1.text, s2 && s2.text);. Also, these functions don't use any closure variables.
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'm accustomed to syntactic scopes conveying encapsulation. If it is more idiomatic for them to express closure, then I agree that these functions should not be nested.
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.
Opinions on that vary around the team, so whichever is fine.
src/services/organizeImports.ts Outdated
| } | ||
| } | ||
| for(constnamedImportofnamedImports){ |
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.
newImportSpecifiers.push(...flatMap(namedImports, n => n.elements));
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.
Thanks, I figured there had to be a function that did that. I was looking for "selectMany".
src/services/organizeImports.ts Outdated
| constgroupedImports=groupSortedImports(sortedImports); | ||
| for(constimportGroupofgroupedImports){ | ||
| letseenImportWithoutClause=false; |
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.
Think this could be its own function:
functiongetImportParts(importGroup:ReadonlyArray<ImportDeclaration>):{importWithoutClause:ImportDeclaration|undefined,defaultImports:ReadonlyArray<Identifier>,namespaceImports:ReadonlyArray<NamespaceImport>,namedImports:ReadonlyArray<NamedImports>,}{letimportWithoutClause:ImportDeclaration;constdefaultImports:Identifier[]=[];constnamespaceImports:NamespaceImport[]=[];constnamedImports:NamedImports[]=[];for(constimportDeclarationofimportGroup){if(importDeclaration.importClause===undefined){// Only the first such import is interesting - the others are redundant.// Note: Unfortunately, we will lose trivia that was on this node.importWithoutClause=importDeclaration;continue;}const{ name, namedBindings}=importDeclaration.importClause;if(name){defaultImports.push(name);}if(namedBindings){if(isNamespaceImport(namedBindings)){namespaceImports.push(namedBindings);}else{namedImports.push(namedBindings);}}}return{ importWithoutClause, defaultImports, namespaceImports, namedImports};}
then:
const{ importWithoutClause, defaultImports, namedImports, namespaceImports}=getImportParts(importGroup);if(importWithoutClause){coalescedImports.push(importWithoutClause);}...proceedasbefore...
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.
Sure.
src/services/organizeImports.ts Outdated
| } | ||
| } | ||
| constsortedImportSpecifiers=stableSort(newImportSpecifiers,(s1,s2)=>{ |
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.
stableSort(newImportSpecifiers,(s1,s2)=>compareIdentifiers(s1.propertyName||s1.name,s2.propertyName||s2.name)||compareIdentifiers(s1.name,s2.name));
src/services/organizeImports.ts Outdated
| continue; | ||
| } | ||
| letnewDefaultImport:Identifier=undefined; |
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.
Identifier | undefined notIdentifier = undefined
src/services/organizeImports.ts Outdated
| constchangeTracker=textChanges.ChangeTracker.fromContext({ host, formatContext}); | ||
| // NB: Stopping before i === 0 | ||
| for(leti=oldImportDecls.length-1;i>0;i--){ |
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.
Could you add a comment explaining why this is in reverse order? I assume this has something to do with trivia?
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.
It's not - I was thinking of something else.
| } | ||
| constoldValidImportDecls=oldImportDecls.filter(importDecl=>getExternalModuleName(importDecl.moduleSpecifier)); | ||
| constoldInvalidImportDecls=oldImportDecls.filter(importDecl=>!getExternalModuleName(importDecl.moduleSpecifier)); |
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.
Not sure this is needed -- we're basically just deleting these and adding them back, could just ignore them entirely and makeoldImportDecls only be the valid ones.
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.
If they are scattered throughout the file, this will group them together at the top.
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.
OK. For the case where you do keep the invalid ones, could use a helper function and writeconst [oldValidImportDecls, oldInvalidImportDecls] = partition(oldImportDecls, isValidImportDeclaration);
| }); | ||
| } | ||
| functiongetExternalModuleName(specifier:Expression){ |
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.
Could have aValidImportDeclaration type that always has a valid specifier, and then not need to use this function or worry aboutundefined results.
interfaceValidImportDeclarationextendsImportDeclaration{moduleSpecifier:StringLiteral;}functionisValidImportDeclaration(node:Node):node isValidImportDeclaration{returnisImportDeclaration(node)&&isStringLiteral(node.moduleSpecifier);}
then:
constoldImportDecls=sourceFile.statements.filter(isValidImportDeclaration);
(We could technically allowNoSubstitutionTemplateLiterals here, but they're actually compile errors so not really valid.)
amcasey commentedFeb 15, 2018
Restarting tests since lint error line numbers didn't match current source. |
amcasey commentedFeb 16, 2018
I am unable to repro the failure |
ghost commentedFeb 16, 2018
@amcasey Might have to do with newlines? |
amcasey commentedFeb 16, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@Andy-MS Conceivably, but it would be strange to have only this one failure. Edit: They're hard-coded to use |
amcasey commentedFeb 16, 2018
We have many files with mixed line endings, but my current hypothesis is that the problem was introduced when I renamed the baseline output directory from "OrganizeImports" to "organizeImports" for consistency with the others. I'll try to purge the old name locally and then force push. |
amcasey commentedFeb 16, 2018
Other than this irritating issue, is this more or less ready to merge? |
In phase 1, it coalesces imports from the same module and sorts theresults, but does not remove unused imports.Some trivia is lost during coalescing, but none should be duplicated.
| renameFilename?:string; | ||
| } | ||
| /** |
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.
This comment might be better inorganizeImports.ts (the non-test one).
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 can move it when I implement remove-unused.
Eliminate cancellation tokenAdd organizeImports.ts to tsconfig.jsonSimplify ts.OrganizeImports.organizeImportsSimplify sortImportsSemantic change: all invalid module specifiers are now considered to beequal.Simplify comparisons using ||Pull out imports with invalid modules specifiers...for separate processing. They are tacked on to the end of theorganized imports in their original order.Bonus: downstream functions can now assume imports have valid modulespecifiers.Rename baseline folder with leading lowercaseSimplify coalesceImportsRemove some unnecessary null checksSimplify baseline generation
amcasey commentedFeb 16, 2018
I've revised history so that the baseline folder has always been lowercase. Hopefully, that will fix the tests. |
MgSam commentedApr 5, 2018
How do you invoke this command in Visual Studio? I've been completely unable to get it to appear anywhere since upgrading to 2.8. |
mhegazy commentedApr 5, 2018
This is a new command, so requires update to VS as well. Should light up in your next VS update. |
In phase 1, it coalesces imports from the same module and sorts the
results, but does not remove unused imports.
Some trivia is lost during coalescing, but none should be duplicated.