-
Notifications
You must be signed in to change notification settings - Fork 747
added gating for user preferences for auto imports #1793
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?
Conversation
…3/typescript-go into auto-imports-user-prefs" This reverts commit 865d44bc8948e7165defb117837464f5cbe2326d, reversing changes made to 186540b5240e2e31e61a2c4a8467d79b8174292c.
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.
Pull Request Overview
This PR adds user preference gating for auto-import functionality in the TypeScript language server. The changes enable configurable behavior for module specifier preferences, import path endings, file/specifier exclusion patterns, type-only auto-imports, and import path renaming.
Key changes include:
- Converting several boolean preferences to
core.Tristatefor consistency (PreferTypeOnlyAutoImports,AllowRenameOfImportPath) - Implementing user preference filtering for auto-imports via
AutoImportFileExcludePatternsandAutoImportSpecifierExcludeRegexes - Adding support for module specifier preferences (
ImportModuleSpecifierPreference,ImportModuleSpecifierEnding) - Exporting
GetSubPatternFromSpecin thevfspackage for pattern matching - Adding comprehensive test coverage for the new preference options
Reviewed Changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
testdata/baselines/reference/fourslash/autoImports/*.baseline.md |
Baseline outputs for new auto-import preference tests |
internal/vfs/utilities.go |
Exported GetSubPatternFromSpec for use in auto-import filtering |
internal/modulespecifiers/types.go |
Added allowRenameOfImportPath field to UserPreferences struct |
internal/modulespecifiers/specifiers.go |
Added baseUrl fallback logic and path component counting function |
internal/lsp/server.go |
Updated rename handler to accept user preferences parameter |
internal/ls/lsutil/userpreferences.go |
Converted preference fields to core.Tristate and removed comment markers |
internal/ls/findallreferences.go |
Added preference gating for import path renaming |
internal/ls/codeactions_importfixes.go |
Passed user preferences to auto-import filtering |
internal/ls/autoimportsexportinfo.go |
Passed user preferences to export info collection |
internal/ls/autoimports.go |
Implemented file and specifier exclusion pattern filtering |
internal/ls/autoimportfixes.go |
Updated to use core.Tristate for type-only preferences |
internal/fourslash/tests/*.go |
Added comprehensive tests for all new preference options |
internal/fourslash/fourslash.go |
Updated test harness to preserve user preferences in baselines |
internal/fourslash/_scripts/failingTests.txt |
Removed now-passing test from failing list |
Comments suppressed due to low confidence (1)
internal/vfs/utilities.go:166
- The
matcherparameter is immediately overwritten at line 166 withwildcardMatchers[usage], making the parameter effectively unused. This means all callers are passing a value that gets ignored.
Either:
- Remove the overwrite at line 166 to use the passed-in
matcherparameter, OR - Remove the
matcherparameter entirely and just usewildcardMatchers[usage]throughout the function (and update all call sites accordingly)
Given that all current call sites pass wildcardMatchers[usage] anyway, option 2 might be simpler and clearer.
func GetSubPatternFromSpec(
spec string,
basePath string,
usage Usage,
matcher WildcardMatcher,
) string {
matcher = wildcardMatchers[usage]
|
Is the test generator just missing these options such that all tests are new handwritten ones? |
| if preferences.AutoImportFileExcludePatterns == nil { | ||
| return nil | ||
| } | ||
| var patterns []*regexp2.Regexp |
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 so wish this feature had not been Regexp based
Isabel mentioned that I'd need to handwrite tests using |
|
Codefixes were ported in #2053 |
Implements and tests User Preferences for Auto Imports; the specific preferences included are shown below.
importModuleSpecifierPreference: "shortest" | "project-relative" | "relative" | "non-relative";importModuleSpecifierEnding: "auto" | "minimal" | "index" | "js";includePackageJsonAutoImports: "auto" | "on" | "off";allowRenameOfImportPath: boolean;autoImportFileExcludePatterns: string[];autoImportSpecifierExcludeRegexes: string[];preferTypeOnlyAutoImports: boolean;