-
Notifications
You must be signed in to change notification settings - Fork 93
Smaller text edits in lsp formatting handler #1031
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
Smaller text edits in lsp formatting handler #1031
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.
Very cool, thanks for taking the time to implement this! Looks pretty reasonable to me.
The only thing I'm a bit wary of is the FormattingOptions. Ignoring this originally was intentional, as I'd prefer project configuration is explicitly all part of the stylua.toml. In practice, the issue we are trying to avoid is if your editors formatting options don't match the formatting options that a project has defined (e.g., you prefer indent with spaces, this is configured in your editor, but you're working on a project that indents with tabs, now the formatting is incorrect as stylua.toml is not respected)
I'd prefer to leave this one out, but if you reckon it's useful I'd be open to making this configurable in some way. Maybe an (off-by-default) option passed in initialization options, like respect_editor_formatting_options? If we go that route, let's also mention it in the README.
Also, could you update the changelog with this improvement?
55b9c2c to
893d1bd
Compare
I agree that explicitly set config options shouldn't be overridden, but if neither The fields in the I know this change is sorta out-of-scope for this PR, but I couldn't think of another way to use |
|
Sorry, I'm still not sure about that. For example, in all of my projects I rely on the stylua defaults and do not explicitly specify them in For now, I'd prefer to leave it out of this change. Everything else LGTM (and thanks for catching the missing range formatting server capability too) |
432f7d9 to
440498d
Compare
440498d to
33fe055
Compare
|
Ok, thank you for considering the idea though. I've replaced the fallback behavior with a new init option |
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.
Thank you!
Hi, I saw the
TODOin the new LSP implementation and thought I'd try it.Now, the formatting handler does a byte by byte diff of the unformatted and formatted contents, and only responds with the changes between the two.
An alternative would be a line by line diff, but I found that doing it byte-wise has two advantages:
document.position_at()can be used to compute positionsAdditionally, I took the liberty of implementing two other nitpicks that I had with the LSP. Let me know if you'd prefer separate PRs or don't want these at all.
Couldn't find a CONTRIBUTING.md, so I hope I didn't miss any PR guidelines 😅.