Skip to content
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

feat: add fixit lsp subcommand #390

Merged
merged 5 commits into from
Apr 17, 2024
Merged

feat: add fixit lsp subcommand #390

merged 5 commits into from
Apr 17, 2024

Conversation

llllvvuu
Copy link
Contributor

@llllvvuu llllvvuu commented Sep 7, 2023

resolves #387 #122

Note: easier to read in unified view since it's new code.
Note: uses the popular pygls (Apache 2.0) implementation of the protocol.

demo.mp4

Summary

Support for:

  • textDocument/didOpen, textDocument/didChange -> textDocument/publishDiagnostics
  • textDocument/formatting

Not supported yet

If this PR gets merged I will create follow-up issues for these items.

Test Plan

Added new smoke test for the new fixit lsp subcommand.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 7, 2023
@llllvvuu llllvvuu force-pushed the feat/lsp branch 2 times, most recently from 70ad9f7 to d4b6989 Compare September 7, 2023 03:50
@llllvvuu llllvvuu changed the title feat(lsp): live diagnostics, fix whole file feat: add fixit lsp subcommand Sep 7, 2023
@llllvvuu llllvvuu force-pushed the feat/lsp branch 7 times, most recently from 2551e15 to 1d748d8 Compare September 10, 2023 22:56
@amyreese
Copy link
Member

Sorry for the delay. I'll try to review this and your other PR's some time next week.

Copy link
Member

@amyreese amyreese left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in reviewing this, and I really appreciate your time in putting this together. My only experience with writing an LSP is using the vscode templates, so forgive me if some of this goes against common patterns. I'm mostly interested in keeping the code more readable and maintainable, and I had a bit of difficulty following what was happening here. 😅

I don't want to impose on your time too much, so just let me know if you'd rather me be the one to adopt these suggestions, and I'll make sure to keep you as the primary author on the PR.

Cheers!

pyproject.toml Outdated Show resolved Hide resolved
src/fixit/cli.py Outdated Show resolved Hide resolved
src/fixit/cli.py Outdated Show resolved Hide resolved
src/fixit/ftypes.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
src/fixit/lsp.py Outdated Show resolved Hide resolved
@llllvvuu
Copy link
Contributor Author

No worries, might be delayed as I don't have Wi-fi this month, but I'm happy to make the changes as they all seem sensible!

@llllvvuu llllvvuu force-pushed the feat/lsp branch 3 times, most recently from d3f16a3 to 6c79b05 Compare November 8, 2023 08:36
@llllvvuu
Copy link
Contributor Author

llllvvuu commented Nov 8, 2023

Updated, sorry for the delay!

@llllvvuu llllvvuu force-pushed the feat/lsp branch 5 times, most recently from 7fdf353 to 9dcd1ec Compare November 8, 2023 08:58
@povilasb
Copy link

This is cool!

Hope it will get merged 🤞

llllvvuu and others added 2 commits April 15, 2024 19:12
Support for:
- [x] `textDocument/didOpen`, `textDocument/didChange` -> `textDocument/publishDiagnostics`
- [x] `textDocument/formatting`

No support yet:

- [ ] `textDocument/codeAction`, `workspace/executeCommand`
- [ ] `workspace/didChangeWatchedFiles` to invalidate the config cache

test: Added new smoke test for the new `fixit lsp` subcommand.
@amyreese amyreese merged commit 802b975 into Instagram:main Apr 17, 2024
16 checks passed
@amyreese
Copy link
Member

Thank you for contributing this work and for responding to feedback. Look forward to this being in a future release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: implement LSP
4 participants