-
Notifications
You must be signed in to change notification settings - Fork 83
Implement the new codelens.forNestedBindings setting
#1990
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
Implement the new codelens.forNestedBindings setting
#1990
Conversation
codelens.forNestedBindings setting.codelens.forNestedBindings setting
Signed-off-by: Sora Morimoto <[email protected]>
Updated the parameter name from `codelens` to `codelens_for_nested_bindings` in the set_configuration functionto enhance code readability and maintain consistency with the naming convention. Signed-off-by: Sora Morimoto <[email protected]>
e192973 to
b8b84bf
Compare
Signed-off-by: Sora Morimoto <[email protected]>
Signed-off-by: Sora Morimoto <[email protected]>
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 implements support for the new codelens.forNestedBindings LSP setting from ocaml-lsp, allowing users to control whether CodeLens appears on nested let bindings. The implementation also includes a breaking change by renaming the existing ocaml.server.codelens setting to ocaml.server.codelens.enable.
Key changes:
- Added
codelens.forNestedBindingssetting with a default value offalse - Renamed existing
ocaml.server.codelenssetting toocaml.server.codelens.enable - Introduced new
OcamllspSettingCodeLensmodule to handle structured codelens configuration
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/settings.ml | Added new setting and updated codelens setting key from ocaml.server.codelens to ocaml.server.codelens.enable |
| src/settings.mli | Added interface declaration for the new nested bindings setting |
| src/vscode_ocaml_platform.ml | Read and pass the new nested bindings setting to the extension instance |
| src/extension_instance.ml | Store and propagate the nested bindings setting to the LSP client configuration |
| src/extension_instance.mli | Added parameter for nested bindings setting in configuration function |
| src/ocaml_lsp.ml | Replaced OcamllspSettingEnable with structured OcamllspSettingCodeLens module for codelens settings |
| src/ocaml_lsp.mli | Updated interface to use the new codelens settings structure |
| package.json | Updated configuration schema with renamed setting and new nested bindings option |
| CHANGELOG.md | Documented the breaking change and new feature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
smorimoto
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.
LGTM
|
There is an incompatibility between older versions of OCaml-lsp-server that do not implement the setting and the version of vscode-ocaml-platform that uses this patch. I'm trying to find a workaround. It seems that this is not due to the difference in naming convention pointed out by Max (#1990 (comment)), even after fixing this, the problem is still present. |
|
The field |
|
Should be correct now, thank you @voodoos 👍 |
Co-authored-by: Max Lantas <[email protected]>
|
Thanks @Tim-ats-d! |
Implement the new setting provided by this PR ocaml/ocaml-lsp#1567