-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Fix triple slash completions #11328
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
Merged
Merged
Fix triple slash completions #11328
Changes from all commits
Commits
Show all changes
36 commits
Select commit
Hold shift + click to select a range
2035db1
Convert to Doc Comment
b38d7ac
Annotate directorySeparator
d423aad
comments
8f883b9
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
1f7b6e6
More comments
769d248
new test
8a479e8
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
6dd5482
remove Comment
bfb2185
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
629e126
Factored out hidden path test
f6ff6bd
factor and simplify rel path test
49e0de9
Fix tests
2195f17
Rename Tests
4b07377
Fix fragment handling
8573631
Rename Tests
cb13ae0
Pass linting
6553413
New Test
a2cbc63
Question added
d96d098
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
b9e5dbc
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
3e15394
add readDirectory to LSHost
3ce97af
Rename labels in tests
728252f
Merge branch 'master' into FixAbsoluteTripleSlashCompletions
179ceda
Removed comments and fixed indentation
0871628
Remove trailing comment
d737dc1
Responding To billti's comments
55d4e12
Fixed a comment
2e589f1
Actually fixed comments
eb362fe
Merge branch 'master' into FixTripleSlashCompletions
1baf496
merge master and add isGlobalCompletion flags to CompletionInfo
a12fe2e
Eliminate allocation of filtered completions
617daa9
Change fragment initialization
24938a7
Remove filtering functionality
77a2d0e
Merge branch 'master' into FixTripleSlashCompletions
4a497fb
update tests to reflect no completion filtering
af833aa
provide completions only in the correct bounds
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 hidden or 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 hidden or 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 hidden or 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 hidden or 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
70 changes: 0 additions & 70 deletions
70
tests/cases/fourslash/completionForStringLiteralRelativeImport1.ts
This file was deleted.
Oops, something went wrong.
72 changes: 72 additions & 0 deletions
72
tests/cases/fourslash/completionForStringLiteralRelativeImportAllowJSFalse.ts
This file contains hidden or 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,72 @@ | ||
| /// <reference path='fourslash.ts' /> | ||
|
|
||
| // Should give completions for ts files only when allowJs is false. | ||
|
|
||
| // @Filename: test0.ts | ||
| //// import * as foo1 from "./*import_as0*/ | ||
| //// import * as foo2 from ".//*import_as1*/ | ||
| //// import * as foo4 from "./d1//*import_as2*/ | ||
|
|
||
| //// import foo6 = require("./*import_equals0*/ | ||
| //// import foo7 = require(".//*import_equals1*/ | ||
| //// import foo9 = require("./d1//*import_equals2*/ | ||
|
|
||
| //// var foo11 = require("./*require0*/ | ||
| //// var foo12 = require(".//*require1*/ | ||
| //// var foo14 = require("./d1//*require2*/ | ||
|
|
||
| // @Filename: d2/d3/test1.ts | ||
| //// import * as foo16 from "..//*import_as3*/ | ||
| //// import foo17 = require("..//*import_equals3*/ | ||
| //// var foo18 = require("..//*require3*/ | ||
|
|
||
|
|
||
| // @Filename: f1.ts | ||
| //// /*f1*/ | ||
| // @Filename: f2.js | ||
| //// /*f2*/ | ||
| // @Filename: f3.d.ts | ||
| //// /*f3*/ | ||
| // @Filename: f4.tsx | ||
| //// /f4*/ | ||
| // @Filename: f5.js | ||
| //// /*f5*/ | ||
| // @Filename: f6.jsx | ||
| //// /*f6*/ | ||
| // @Filename: f7.ts | ||
| //// /*f7*/ | ||
| // @Filename: d1/f8.ts | ||
| //// /*d1f1*/ | ||
| // @Filename: d1/f9.ts | ||
| //// /*d1f9*/ | ||
| // @Filename: d2/f10.ts | ||
| //// /*d2f1*/ | ||
| // @Filename: d2/f11.ts | ||
| //// /*d2f11*/ | ||
|
|
||
| const kinds = ["import_as", "import_equals", "require"]; | ||
|
|
||
| for (const kind of kinds) { | ||
| goTo.marker(kind + "0"); | ||
| verify.completionListIsEmpty(); | ||
|
|
||
| goTo.marker(kind + "1"); | ||
| verify.completionListContains("f1"); | ||
| verify.completionListContains("f3"); | ||
| verify.completionListContains("f4"); | ||
| verify.completionListContains("f7"); | ||
| verify.completionListContains("d1"); | ||
| verify.completionListContains("d2"); | ||
| verify.not.completionListItemsCountIsGreaterThan(6); | ||
|
|
||
| goTo.marker(kind + "2"); | ||
| verify.completionListContains("f8"); | ||
| verify.completionListContains("f9"); | ||
| verify.not.completionListItemsCountIsGreaterThan(2); | ||
|
|
||
| goTo.marker(kind + "3"); | ||
| verify.completionListContains("f10"); | ||
| verify.completionListContains("f11"); | ||
| verify.completionListContains("d3"); | ||
| verify.not.completionListItemsCountIsGreaterThan(3); | ||
| } |
Oops, something went wrong.
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.
Isn't this doing the same thing as line 330?
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 function really should never get
undefinedas an argument for fragment, but I don't know of a way to assert that. So the check on line 330 is really a matter of null-checking. In particular,normalizeSlashes()on line 332 dereferences its argument without null-checking.Regarding line 340, consider the case when
fragmentis"foo". ThengetDirectoryPath(fragment) === ""because it removes the basename part of the path. So we need to check again.Really,
fragmentno longer refers to a path fragment after the assignment on line 337. Perhaps a new variable calledrelativeOrAbsolutePathshould be initialized on the LHS of line 337. I didn't make the change because I was trying to minimize the changes I was making to the codebase to get a fix in (with mixed success).Should I add a new declaration?
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, the comment was specifically about the fact that line 330 uses
"./"while line 340 uses"." + directorySeparator", though they do the same thing. Perhaps you should initializefragmentto""on 330 rather than repeat yourself on 340.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.
Changed.