Format options + fourslash testing + fix formatting bugs #2370
Conversation
…o formatOptions
…o formatOptions
…o formatOptions
iisaduan
requested review from
DanielRosenwasser,
Copilot,
gabritto and
jakebailey
There was a problem hiding this comment.
Pull request overview
This PR adds formatting options support and integrates it with the language server, enables fourslash testing for formatting functionality, and fixes a formatting bug related to extra spaces after newlines.
Key changes:
- Adds
FormatCodeSettingsstructure with comprehensive formatting options - Refactors configuration management to support both TypeScript and JavaScript scoped settings
- Fixes scanner bug that was calculating end-of-line positions incorrectly
- Updates fourslash test infrastructure to support formatting tests
- Reduces baseline diffs showing improved formatting convergence with TypeScript
Reviewed changes
Copilot reviewed 283 out of 283 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
internal/scanner/scanner.go |
Fixed off-by-one error in GetECMAEndLinePosition to correctly calculate line endings |
internal/project/configuration.go |
New configuration structure supporting separate TypeScript/JavaScript preferences |
internal/project/snapshot.go |
Refactored to use pointer-based Config and removed embedded format options |
internal/project/session.go |
Updated to manage Config objects instead of bare UserPreferences |
internal/ls/lsutil/formatcodeoptions.go |
New comprehensive formatting options with parsing and default values |
internal/ls/lsutil/userpreferences.go |
Integrated FormatCodeSettings into UserPreferences structure |
internal/ls/format.go |
Updated to use new format options from UserPreferences |
internal/lsp/server.go |
Enhanced configuration request to support multiple scopes (typescript/ts/javascript/js) |
| Test files | Multiple fourslash tests updated to use new verification methods |
| Baseline files | Reduced diff files showing formatting convergence improvements |
| func (f *FourslashTest) SetFormatOption(t *testing.T, optionName string, value any) { | ||
| t.Helper() | ||
| newPreferences := f.userPreferences.Copy() | ||
| newPreferences.Set(optionName, value) |
There was a problem hiding this comment.
I don't particularly setting these as strings; can we generate these differently so we can stay static? My options refactor will remove any sort of string-based setting
| End: currentCaretPosition, | ||
| Start: currentCaretPosition, |
| Section: ptrTo("typescript"), | ||
| }, | ||
| { | ||
| Section: ptrTo("ts"), |
There was a problem hiding this comment.
The actual section is ts/js, but we should just ignore those for now since we don't pull inferred project compiler options out of it yet.
| } else if settings, ok := params.Settings.([]any); ok { | ||
| s.session.Configure(project.ParseConfiguration(settings)) | ||
| } else if settings, ok := params.Settings.(map[string]any); ok { | ||
| s.session.Configure(project.ParseConfiguration([]any{settings["typescript"], settings["ts"], settings["javascript"], settings["js"]})) |
There was a problem hiding this comment.
If we want to respond to this, we're going to have to also register these in handleInitialized
| } | ||
|
|
||
| // any non-nil field in b is copied into a | ||
| func (a *Config) CopyInto(b *Config) *Config { |
There was a problem hiding this comment.
Do we need mutability for any of the prefs like this? I had thought the change I made before to make it all immutable worked pretty well
| s.configRWMu.Lock() | ||
| defer s.configRWMu.Unlock() | ||
| return s.userPreferences.Copy() | ||
| return s.workspaceConfig.Ts.Copy() |
There was a problem hiding this comment.
What is the plan for returning the "right" one?
If we aren't handling that yet, can we just skip doing the JS stuff for the time being?
2 participants
Adds formatting options and links up the server's
FormatCodeOptionswith the formatting functions. Also adds in basic structure forjs,javascript,ts, andtypescriptscoped settings.Moves
formatCodeSettingstolsutilFormatting is now being tested in fourslash, although not all the commands are implemented due to non-formatting tests crashing on insertion. I've added a fourslashTest property to skip the check for those specific crashes, so that the rest of the test can proceed as normal. The flag(s) can easily be commented out if investigating these crashes.
Also fixes a formatting bug that was adding extra spaces after newlines.