Added alphabetization logic to missing import quick fixes #21876

JoshuaKGoldberg · 2018-02-12T05:42:47Z

When generating a codefix to add a missing import to an imports list, instead of always adding to the end, the node will be inserted before the first node its name is < alphabetically (or at the end if < none). Linear search is used instead of binary as the (generally few) nodes are not guaranteed to be in order.

Fixes #16119

I think I'll need it for inserting a node before the first in a list.

Searches through the elements until one with a "greater" (using native string compares) one is found, and inserts at that position. Test cases 8, 10, and 11 are failing. Copied them as 13, 14, and 15 for inserting in the middle of a list. Copied them as 16, 17, and 18 for inserting at the end of a list.

* Mismatched test case commas in 8, 12, 13, 16 * Mismatched test case endlines in 11, 15 * Was using `sampleIndex - 1` but passing `0` as that index for the before-first insertion. Switched to just using `sampleIndex`. * Used last node instead of end of the first node for the ending range when determining line separator style for inserting after the first. * Inserting before the first in a multiline list needs to insert at position - indentation Also forgot to `return this;` at the end.

Public first, then private.

mhegazy · 2018-02-12T19:00:42Z

@amcasey and @Andy-MS do you mind reviewing this change?

ghost · 2018-02-12T19:23:40Z

src/services/textChanges.ts

+         * Linear search is used instead of binary as the (generally few) nodes are not guaranteed to be in order.
+         */
+        public insertNodeInListAlphabetically(sourceFile: SourceFile, containingList: NodeArray<ImportSpecifier>, newNode: ImportSpecifier): this {
+            if (newNode.name.text < containingList[0].name.text) {


The way we're using this, containingList will contain at least one element, but it's not obvious if you were just looking at this method.
This may be too specific to belong in textChanges and could just go in importFixes. Though I think insertNodeBefore could be adapted so importFixes doesn't have to call a special insertNodeInListBeforeFirst method or have to call insertNodeInListAfter on a previous element.

ghost · 2018-02-12T19:30:10Z

src/services/textChanges.ts

+            }
+
+            for (let i = 1; i < containingList.length; i += 1) {
+                if (newNode.name.text < containingList[i].name.text) {


Not sure, but perhaps compareStringsCaseInsensitive would be appropriate?

amcasey · 2018-02-12T23:10:51Z

What is the sort order of an aliased imported symbol like a as z?

JoshuaKGoldberg · 2018-02-12T23:22:41Z

In this PR, it goes by the .name of the import. So a as z would become z.

mhegazy · 2018-02-12T23:24:57Z

We need to use #21909 for this change.

JoshuaKGoldberg · 2018-02-12T23:25:16Z

I'll wait :)

Ghabriel · 2018-02-27T17:05:13Z

#21909 has been merged. What's the status of this pull request?

JoshuaKGoldberg · 2018-02-27T17:20:49Z

Thanks for the ping @Ghabriel - I'll take a stab at resolving the merge conflicts hopefully this week.

JoshuaKGoldberg · 2018-03-21T17:42:59Z

@Andy-MS have I addressed your comments?

# Conflicts: # src/services/textChanges.ts

Ghabriel · 2018-05-23T20:25:59Z

@amcasey @mhegazy @Andy-MS can you merge this?

amcasey

Still a few concerns.

amcasey · 2018-06-01T22:55:48Z

src/services/codefixes/importFixes.ts

+    /**
+     * This function should be used to insert nodes alphabetically in lists when nodes don't carry separators as the part of the node range,
+     * e.g. export lists, import lists, etc.
+     * Linear search is used instead of binary as the (generally few) nodes are not guaranteed to be in order.


I would have thought there was nothing to do if the nodes were not already in order. Would it make sense to do a pre-pass to check whether they're in order then binary search if they are and add to the end if they're not?

amcasey · 2018-06-01T22:57:51Z

src/services/codefixes/importFixes.ts

+     */
+    function insertNodeInsideListNodesAlphabetically(context: SymbolContext, sourceFile: SourceFile, containingList: NodeArray<ImportSpecifier>, newNode: ImportSpecifier) {
+        return ChangeTracker.with(context, t => {
+            if (compareStringsCaseInsensitive(newNode.name.text, containingList[0].name.text) === Comparison.LessThan) {


I can see why either the first iteration or the last iteration has to be special, but not why both need to be pulled out.

amcasey · 2018-06-01T23:04:40Z

src/services/textChanges.ts

@@ -483,107 +483,150 @@ namespace ts.textChanges {
            if (index < 0) {
                return this;
            }
-            const end = after.getEnd();
            if (index !== containingList.length - 1) {
                // any element except the last one
                // use next sibling as an anchor
                const nextToken = getTokenAtPosition(sourceFile, after.end, /*includeJsDocComment*/ false);
                if (nextToken && isSeparator(after, nextToken)) {


I don't think this actually changed, but does the insertion just silently fail if there's no next token or it's not a separator?

amcasey · 2018-06-01T23:08:20Z

src/services/textChanges.ts

+            }
+        }
+
+        public insertNodeInListBeforeFirst(sourceFile: SourceFile, containingList: NodeArray<Node>, first: Node, newNode: Node): void {


Will first always be containingList[0]?

amcasey · 2018-06-01T23:36:08Z

src/services/codefixes/importFixes.ts

+     */
+    function insertNodeInsideListNodesAlphabetically(context: SymbolContext, sourceFile: SourceFile, containingList: NodeArray<ImportSpecifier>, newNode: ImportSpecifier) {
+        return ChangeTracker.with(context, t => {
+            if (compareStringsCaseInsensitive(newNode.name.text, containingList[0].name.text) === Comparison.LessThan) {


I'd be happier if this were more tightly coupled to organizeImports so that they don't get out of sync. Maybe we could export compareIdentifiers from organizeImports.ts and use it here?

amcasey · 2018-06-01T23:37:20Z

src/services/textChanges.ts

+            let separator: SyntaxKind.CommaToken | SyntaxKind.SemicolonToken;
+            let multilineList = false;
+
+            // insert element after the last element in the list that has more than one item


I'm not sure this comment makes sense here. Does it need to be moved or rephrased?

amcasey · 2018-06-01T23:40:22Z

src/services/textChanges.ts

+            const afterStart = after.getStart(sourceFile);
+            const afterStartLinePosition = getLineStartPositionForPosition(afterStart, sourceFile);
+
+            const { multilineList, separator } = this.getMultilineAndSeparatorOfList(sourceFile, containingList, afterStartLinePosition, index - 1, after);


amcasey · 2018-06-01T23:41:56Z

src/services/textChanges.ts

+                const afterMinusOneStartLinePosition = getLineStartPositionForPosition(containingList[sampleIndex].getStart(sourceFile), sourceFile);
+                multilineList = afterMinusOneStartLinePosition !== afterStartLinePosition;
+            }
+            if (hasCommentsBeforeLineBreak(sourceFile.text, sampleNode.end)) {


!multilineList &&?

amcasey · 2018-06-01T23:43:46Z

src/services/textChanges.ts

+        }
+
+        public insertNodeInListBeforeFirst(sourceFile: SourceFile, containingList: NodeArray<Node>, first: Node, newNode: Node): void {
+            const startPosition = first.getStart(sourceFile);


It looks like this might be after leading trivia on first. Does that matter?

amcasey · 2018-06-01T23:45:54Z

src/services/textChanges.ts

+            }
+
+            const last = containingList[containingList.length - 1];
+            const indentation = formatting.SmartIndenter.findFirstNonWhitespaceColumn(afterStartLinePosition, last.end, sourceFile, this.formatContext.options);


Why continue until last.end? Isn't startPosition sufficient?

RyanCavanaugh · 2018-09-05T16:01:47Z

@JoshuaKGoldberg can you follow up with Andrew's comments/questions? Thanks!

JoshuaKGoldberg · 2018-09-05T16:24:42Z

Will try to get to this soon, thanks for the ping! I'll close the issue if I don't think I'll have time within a month.

JoshuaKGoldberg · 2018-09-29T22:53:41Z

😢

Josh Goldberg added 7 commits February 10, 2018 11:34

Refactored insertNodeInListAfter in preparation for alphabetizing

99b04c7

Moved end into ~AfterLast (slightly simpler this way)

94bffe0

Moved getMultilineAndSeparatorOfList into its own function

a361cb1

I think I'll need it for inserting a node before the first in a list.

Fixed remaining test cases :)

372b73e

Reordered textChanges insertion methods

b8d284e

Public first, then private.

ghost reviewed Feb 12, 2018

View reviewed changes

amcasey mentioned this pull request Feb 12, 2018

Introduce an organizeImports command #21909

Merged

Josh Goldberg added 3 commits March 3, 2018 17:36

Merge branch 'master' into sorted-import-fix

aae5528

Feedback: compareStringsCaseInsensitive; function name

ed400e3

Moved insertNodeInsideListNodesAlphabetically to importFixes

c20b81d

Merge branch 'master' into sorted-import-fix

73da772

# Conflicts: # src/services/textChanges.ts

mhegazy requested a review from amcasey May 30, 2018 22:21

amcasey suggested changes Jun 1, 2018

View reviewed changes

RyanCavanaugh added this to the Community milestone Sep 17, 2018

JoshuaKGoldberg closed this Sep 29, 2018

JoshuaKGoldberg deleted the sorted-import-fix branch March 22, 2022 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added alphabetization logic to missing import quick fixes #21876

Added alphabetization logic to missing import quick fixes #21876

JoshuaKGoldberg commented Feb 12, 2018

mhegazy commented Feb 12, 2018

ghost Feb 12, 2018

ghost Feb 12, 2018

amcasey commented Feb 12, 2018

JoshuaKGoldberg commented Feb 12, 2018

mhegazy commented Feb 12, 2018

JoshuaKGoldberg commented Feb 12, 2018

Ghabriel commented Feb 27, 2018

JoshuaKGoldberg commented Feb 27, 2018 •

edited

Loading

JoshuaKGoldberg commented Mar 21, 2018

Ghabriel commented May 23, 2018

amcasey left a comment

amcasey Jun 1, 2018 •

edited

Loading

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

amcasey Jun 1, 2018

RyanCavanaugh commented Sep 5, 2018

JoshuaKGoldberg commented Sep 5, 2018

JoshuaKGoldberg commented Sep 29, 2018

Added alphabetization logic to missing import quick fixes #21876

Added alphabetization logic to missing import quick fixes #21876

Conversation

JoshuaKGoldberg commented Feb 12, 2018

mhegazy commented Feb 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amcasey commented Feb 12, 2018

JoshuaKGoldberg commented Feb 12, 2018

mhegazy commented Feb 12, 2018

JoshuaKGoldberg commented Feb 12, 2018

Ghabriel commented Feb 27, 2018

JoshuaKGoldberg commented Feb 27, 2018 • edited Loading

JoshuaKGoldberg commented Mar 21, 2018

Ghabriel commented May 23, 2018

amcasey left a comment

Choose a reason for hiding this comment

amcasey Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanCavanaugh commented Sep 5, 2018

JoshuaKGoldberg commented Sep 5, 2018

JoshuaKGoldberg commented Sep 29, 2018

JoshuaKGoldberg commented Feb 27, 2018 •

edited

Loading

amcasey Jun 1, 2018 •

edited

Loading