-
Notifications
You must be signed in to change notification settings - Fork 734
Move change tracker, converters, utils to separate packages #1977
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
Conversation
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 reorganizes the codebase by moving several utility packages into more focused locations to improve modularity and prepare for future auto-import work. The main changes extract converters, change tracking, and utility functions into separate packages under internal/ls/.
Key changes:
- Moved converter functionality to
internal/ls/lsconvpackage - Moved change tracker to
internal/ls/changepackage - Moved utility functions to
internal/ls/lsutilpackage - Moved utility functions from
internal/ls/utilities.gotointernal/lsp/lsproto/util.goandinternal/ast/utilities.go
Reviewed Changes
Copilot reviewed 29 out of 31 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/ls/converters.go | Moved to lsconv package |
| internal/ls/linemap.go | Moved to lsconv package |
| internal/ls/converters_test.go | Updated package name to lsconv_test |
| internal/ls/changetracker.go | Moved to change package, renamed changeTracker to Tracker |
| internal/ls/changetrackerimpl.go | Moved to change package, updated method receivers |
| internal/ls/lsutil/utilities.go | New file with probablyUsesSemicolons function moved from ls |
| internal/lsp/lsproto/util.go | New file with ComparePositions and CompareRanges functions |
| internal/ast/utilities.go | Added IsRightSideOfPropertyAccess function |
| internal/ast/symbol.go | Added IsExternalModule and IsStatic methods |
| Multiple files | Updated import paths to reflect new package structure |
jakebailey
left a comment
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.
Seems good, though I am not sure if the name change will be annoying or not given the genericness of that name.
jakebailey
left a comment
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.
lsutils seems like a weird home for UserPreferences; makes me think that this whole thing should be inverted somehow
|
I feel like UserPreferences : something :: DocumentURI : lsproto, so I think it makes sense for it to go in a lower-level, dependency-light package that can be depended upon by various bits of LS functionality. I agree |
|
If every LS feature moved into its own package (not unreasonable, but a big lift at the moment), that would free up |
|
Yeah. I wasn't intending to imply something actionable here, I definitely see where this is going and it's all good! |
I started down this path while working on auto-imports and pulled it out to a standalone PR as it's likely to cause conflicts.