-
Notifications
You must be signed in to change notification settings - Fork 682
[DRAFT] autoimport #1553
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
base: main
Are you sure you want to change the base?
[DRAFT] autoimport #1553
Conversation
I'm gonna closely follow this one... |
bug squashing atm :). Hopefully it'll be ready soon |
@@ -95,8 +98,8 @@ type completionDataData struct { | |||
isJsxIdentifierExpected bool | |||
isRightOfOpenTag bool | |||
isRightOfDotOrQuestionDot bool | |||
importStatementCompletion any // !!! | |||
hasUnresolvedAutoImports bool // !!! | |||
importStatementCompletion *importStatementCompletionInfo // !!! |
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 can drop the // !!!
here now I think
// Unless this option is `false`, member completion lists triggered with `.` will include entries | ||
// on potentially-null and potentially-undefined values, with insertion text to replace | ||
// preceding `.` tokens with `?.`. | ||
IncludeAutomaticOptionalChainCompletions *bool | ||
|
||
// If enabled, the completion list will include completions with invalid identifier names. | ||
// For those entries, The `insertText` and `replacementSpan` properties will be set to change from `.x` property access to `["x"]`. | ||
IncludeCompletionsWithInsertText *bool |
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.
We don't need this anymore, every LSP client should support this.
moduleSymbol, | ||
file, | ||
position, | ||
"", // entryId.data.exportMapKey |
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.
What are these comments for? Is it the corresponding Strada code?
} | ||
|
||
// If some file is using ES6 modules, assume that it's OK to add more. | ||
return true || // !!! symlink program.getSymlinkCache?.().hasAnySymlinks() || |
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.
Shouldn't this be false
while the symlink condition isn't implemented yet?
@@ -731,7 +731,7 @@ func (f *FourslashTest) verifyCompletionsAreExactly(t *testing.T, prefix string, | |||
var completionIgnoreOpts = cmp.FilterPath( | |||
func(p cmp.Path) bool { | |||
switch p.Last().String() { | |||
case ".Kind", ".SortText", ".Data": |
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.
We have completion tests that test filterText
, so we shouldn't always ignore it. If you need to ignore it for some tests, I'd suggest adding an explicit ignore
option like we do for other properties (see ExpectedCompletionEditRange
). I think ideally we'd do what we did in Strada where we checked if the property was defined, but since that's not a thing in Go it's trickier to allow for testing presence/absence of filterText
when we want to but ignore it the rest of the time. If it's just for a few tests, we can probably manually fill in filterText
, otherwise we can set up an explicit ignore
. I've been meaning to do that for a while now, let me know if you want me to do that and what tests are failing without this change it.
@@ -2428,6 +2428,10 @@ func GetLineStarts(sourceFile ast.SourceFileLike) []core.TextPos { | |||
return sourceFile.LineMap() | |||
} | |||
|
|||
func GetLineStartPositionForPosition(pos int, lineMap []core.TextPos) int { |
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 if it does what you need, but we have a function with the same name in internal/format/util.go
.
return true | ||
} | ||
|
||
var prevChar *rune |
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 suspect you might be able to get away with using 0
here, given an identifer can't contain NUL
.
@iisaduan thanks a lot for the reply. This is history in the making! Lol |
in-progress port of autoimport completions
also going to do some code cleanup/reorganizing, I wasn't sure how much code each thing would have when i made the files/chose which file to put things in in the first place