-
Notifications
You must be signed in to change notification settings - Fork 800
Format options + fourslash testing + fix formatting bugs #2370
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
…o formatOptions
…o formatOptions
…o formatOptions
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 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 |
| Section: ptrTo("typescript"), | ||
| }, | ||
| { | ||
| Section: ptrTo("ts"), |
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.
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.
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 would still delete ts and js here, I don't think they are real?
…o formatOptions
…o formatOptions
| "github.com/microsoft/typescript-go/internal/ls/lsutil" | ||
| ) | ||
|
|
||
| type Config struct { |
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 think Config is a bit too generic of a name, and it does seem like this should live in lsutil since it's basically just a way to encode two user preferences structs and that isn't project specific.
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.
ServerConfig?
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.
CombinedUserPreferences? AllUserPreferences? I didn't come up with a good name so I didn't suggest one before, which is not helpful 😄
My hope is that in a future change, this struct is immutable too (probably in a rebase of my PR), just to make this easier to work with
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 had originally imagined that we would factor out the stuff configures server side preferences (stuff that would never need to be passed into LanguageService). There's also tsserverOptions in strada, I figured we'd have similar options here (or are they all just on Server already?
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.
Maybe I'm totally missing something, but the only reference to tsserverOptions is in a comment in Corsa...
Adds formatting options and links up the server's
FormatCodeOptionswith the formatting functions. Also adds in basic structure forjs,javascript,ts, andtypescriptscoped settings, and redirectsLanguageService.UserPreferences()to return the preferences for the active file.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.
Fixes #2450