Improve handling of public symbols for imports #1178
Conversation
|
Nice! And thanks for providing the unit tests too. |
|
@heejaechang, could you please review this change? |
cdce8p
force-pushed
the
import-symbols
branch
from
56595d8
to
d5b7725
Compare
| @@ -279,11 +279,11 @@ export class AutoImporter { | |||
|
|
|||
| if ( | |||
| !isStubOrHasInit.isStub && | |||
| autoImportSymbol.kind === CompletionItemKind.Variable && | |||
| /[a-z]/.test(name) | |||
| (autoImportSymbol.kind === undefined || autoImportSymbol.kind === CompletionItemKind.Variable) && | |||
There was a problem hiding this comment.
which Symbol Kind is "autoImportSymbol.kind === undefined" for?
There was a problem hiding this comment.
I am asking since now it will check every symbol rather than only variable symbol kind.
There was a problem hiding this comment.
I think what you want is tweaking this (
), not kind === undefined.There was a problem hiding this comment.
existing code basically says, if pyright already figured out that variable is "Final" or "Const", then we will always show it in auto-import regardless of how the name is written (there is some corner cases if it is from indexer, but we can ignore that one for now)
There was a problem hiding this comment.
I see. Originally I noticed that protected constants where also showing up, which is why I added that check. However after you mentioning it, I now believe that it might be for the better the way constants are currently handled. Unlike with variables, there are at least some scenarios in which you might what to import them.
I'll change it.
| autoImportSymbol.kind === CompletionItemKind.Variable && | ||
| /[a-z]/.test(name) | ||
| (autoImportSymbol.kind === undefined || autoImportSymbol.kind === CompletionItemKind.Variable) && | ||
| !SymbolNameUtils.isPublicConstantOrTypeAlias(name) |
There was a problem hiding this comment.
previously, name check was negative check based on the fact that the lexer has already used proper identifier check (https://github.com/microsoft/pyright/blob/master/packages/pyright-internal/src/parser/characters.ts#L52) which allows non enlgish unicode as variable name.
now, only English name can be used in auto import variable. probably okay? but want to make sure that everyone is okay with that.
ex) before one could have 검은색상값 = "BLACK" and it will show up in auto import, now it won't.
There was a problem hiding this comment.
@heejaechang, good question. Are you familiar with the naming conventions used in locales that don't use Roman (ASCII) characters? Would it be common to see someone use "검은색상값" as the name of a type alias, or would that be unexpected and atypical? If it's expected, then is there a common heuristic we could use here? How about "starts with a capital letter and has no underscores?"
There was a problem hiding this comment.
I think using non-roman character as a variable name is not common (in Korea at least) since usually, tool support is so bad even if language spec allows it :) but that being said, I believe in some countries (one of them is Japan I believe), it is used more often.
but, this is only for auto-import, but not actual variable name, so might be okay?
There was a problem hiding this comment.
OK, sounds good to me. Let's stick with this for now. If it's too restrictive, I'm sure we'll get feedback.
…added a fourslash test for it. (microsoft#1178) * added partial stub package fourslash test and initial work to map back mapped file path to original file path. currenly, only go to definition and find all references (rename) is handled. * added reverse map and rename * revert back converting code * made test pass * make sure all incoming and outgoing LSP requests are converted to mapped and original file paths. this is done through convertPathToUri and convertUriToPath since all LSP request is based on uril but internally we deal with everything as local filepath. * fix prettier issue * handle partial stub file opened out of band * make sure getOriginalFilePath/MapFilePath to follow the chain and PR feedback * clean ups * call convertUriToPath rather than GetMppaedFile * fixed merge conflicts * move workspaceEdits its own file * remove mapped file concept from file system and get it directly from pyrightFS * fixed build error
See: microsoft/pylance-release#606