-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Fix organize imports inserting extra blank lines #41927
Closed
Closed
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit
Hold shift + click to select a range
cb05c28
Update package-lock.json
typescript-bot 2f4d4c1
Update package-lock.json
typescript-bot b94544a
Update package-lock.json
typescript-bot 60d63dd
Update package-lock.json
typescript-bot 3d45ac0
Update package-lock.json
typescript-bot 7dda7e3
Update package-lock.json
typescript-bot 24f0d6b
Update package-lock.json
typescript-bot a9a8cbb
Update package-lock.json
typescript-bot 9e42a69
Update package-lock.json
typescript-bot 12cc980
Update package-lock.json
typescript-bot a43ec41
Update package-lock.json
typescript-bot 3a6a36a
Update package-lock.json
typescript-bot 202873a
Update package-lock.json
typescript-bot 78ea60a
Update package-lock.json
typescript-bot 2b0578d
Update package-lock.json
typescript-bot e7c832a
Update package-lock.json
typescript-bot 62604bc
Update package-lock.json
typescript-bot 4d807de
Update package-lock.json
typescript-bot dda079d
Update package-lock.json
typescript-bot 9c121aa
Update package-lock.json
typescript-bot 8777dc7
Update package-lock.json
typescript-bot f1aa0f9
Update package-lock.json
typescript-bot e4e4cf2
Update package-lock.json
typescript-bot dd79363
Update package-lock.json
typescript-bot 2e93e31
Merge remote-tracking branch 'upstream/master'
armanio123 c709f17
More sophisticated check for source position comparability
andrewbranch 67ad6b4
Merge branch 'master' of https://github.com/armanio123/TypeScript
armanio123 5215977
Fix organize imports by looking at the nodes positions
armanio123 7cdbdb9
Merge remote-tracking branch 'upstream/master' into FixOrganizeImports
armanio123 b51345e
Rollback formatting changes
armanio123 662ee6c
Added tests, fixed organizeImports algorithm
armanio123 aaf353e
Fix autoformatting again
armanio123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/// <reference path="fourslash.ts" /> | ||
|
||
// Regression test for bug #41417 | ||
|
||
//// import { | ||
//// d, d as D, | ||
//// c, | ||
//// c as C, b, | ||
//// b as B, a | ||
//// } from './foo'; | ||
//// import { | ||
//// h, h as H, | ||
//// g, | ||
//// g as G, f, | ||
//// f as F, e | ||
//// } from './foo'; | ||
//// | ||
//// console.log(a, B, b, c, C, d, D); | ||
//// console.log(e, f, F, g, G, H, h); | ||
|
||
verify.organizeImports( | ||
`import { | ||
a, b, | ||
b as B, c, | ||
c as C, d, d as D, e, f, | ||
f as F, g, | ||
g as G, h, h as H | ||
} from './foo'; | ||
|
||
console.log(a, B, b, c, C, d, D); | ||
console.log(e, f, F, g, G, H, h);` | ||
); |
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.
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 would be broken by spaces between the node end and the list separator, or by duplicate list separators:
Foo
’s end is 16, butBar
’s pos is 20. 17–19 don’t appear in the span of either. This is why I speculated that you might have to look at tokens between nodes to see if they’re just list separators. Or maybe you can do something similar to what’s ingetNodeAtPosition
withforEachChild
to handle this... that is, it should be easy to tell that the position you were given falls between two nodes, and you could choose to return the next one instead of the common parent. If you implement this correctly, I believe you should be able to drop the+ 1
, which indicates the assumption that the list separator occurs immediately afterpreviousNode
and is exactly one character long.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, whatever function this ends up using, it’s likely to be more expensive than the next couple conditions in this function, so this calculation should be moved to be the last.
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.
Makes sense. I have changed the algorithm for instead navigating the parent until the next specifier is found. I have limited to only look in the case the
previousNode
is an import specifier. Only for not affecting other scenarios.Let me know what you think.
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 pretty confident in this logic being universal, so it would be nice to make it work for other scenarios and to keep such specific logic out of the emitter. To make sure I wasn’t giving you bad advice, I prototyped what I had in mind really quick and it seems to work, though the function isn’t beautiful. Here’s my attempt: andrewbranch@c40b579
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.
One alternative more along the lines of what you have now would be to write a utility function that returns the index of a node in its parent’s list-like container. It would have to be a pretty big switch/case to determine what kind of node the parent is to access the right property. It would be more lines of code, but maybe a bit cleaner and more efficient than what I have here. A downside would be that it would need to be kept up to date when new node kinds are added in the future, but that doesn’t happen too often.