Skip to content

Conversation

imnotpoz
Copy link
Contributor

@imnotpoz imnotpoz commented Jul 23, 2025

rebased #1019 on v0.8 and added cmake and glsl

Sanity Checking

  • I have updated the changelog as per my changes
  • I have tested, and self-reviewed my code
  • My changes fit guidelines found in hacking nvf
  • Style and consistency
    • I ran Alejandra to format my code (nix fmt)
    • My code conforms to the editorconfig configuration of the project
    • My changes are consistent with the rest of the codebase
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas
    • I have added a section in the manual
    • (For breaking changes) I have included a migration guide
  • Package(s) built:
    • .#nix (default package)
    • .#maximal
    • .#docs-html (manual, must build)
    • .#docs-linkcheck (optional, please build if adding links)
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

Add a 👍 reaction to pull requests you find important.

@imnotpoz imnotpoz mentioned this pull request Jul 23, 2025
17 tasks
@imnotpoz imnotpoz changed the title cmake and arduino support cmake, arduino and glsl support Jul 23, 2025
@imnotpoz imnotpoz force-pushed the cmake branch 3 times, most recently from 0db94bb to 49cddc5 Compare July 24, 2025 16:51
github-actions bot pushed a commit that referenced this pull request Aug 2, 2025
Copy link

github-actions bot commented Aug 2, 2025

🚀 Live preview deployed from 34e42a6

View it here:

Debug Information

Triggered by: imnotpoz

HEAD at: cmake

Reruns: 1322

github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
@imnotpoz imnotpoz force-pushed the cmake branch 2 times, most recently from 5ab9f44 to 8cbebb0 Compare August 27, 2025 09:47
github-actions bot pushed a commit that referenced this pull request Aug 27, 2025
@imnotpoz
Copy link
Contributor Author

imnotpoz commented Aug 27, 2025

addressed all the remaining comments
I tested the arduino LSP again after rebasing and all the changes, with clangdPackage and cliPackage set to the default values as well as null, in both cases it works with no issues

I also dug a bit into the arduino-cli sources and found that the config path has actually been $HOME/.arduino15/arduino-cli.yaml since at least 2020 (docs/getting-started.md, line 13) so I updated the default config path to that

the last change is making fqbn not nullable and removing the default value, as the LSP needs that to function and I don't think we can assume any default there

@imnotpoz imnotpoz requested a review from horriblename August 27, 2025 18:53
Comment on lines +23 to +43
cmd =
[
(getExe pkgs.arduino-language-server)
"-clangd"
(
if cfg.lsp.clangdPackage == null
then "clangd"
else getExe' cfg.lsp.clangdPackage "clangd"
)
"-cli"
(
if cfg.lsp.cliPackage == null
then "arduino-cli"
else getExe cfg.lsp.cliPackage
)
"-cli-config"
cfg.lsp.cliConfigPath
"-fqbn"
cfg.lsp.fqbn
]
++ cfg.lsp.extraArgs;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cmd =
[
(getExe pkgs.arduino-language-server)
"-clangd"
(
if cfg.lsp.clangdPackage == null
then "clangd"
else getExe' cfg.lsp.clangdPackage "clangd"
)
"-cli"
(
if cfg.lsp.cliPackage == null
then "arduino-cli"
else getExe cfg.lsp.cliPackage
)
"-cli-config"
cfg.lsp.cliConfigPath
"-fqbn"
cfg.lsp.fqbn
]
++ cfg.lsp.extraArgs;
cmd =
[
(getExe pkgs.arduino-language-server)
"-clangd"
(getExe' pkgs.clang-tools "clangd")
"-cli"
(getExe pkgs.arduino-cli)
"-cli-config"
"$HOME/.arduino15/arduino-cli.yaml"
"-fqbn"
cfg.lsp.fqbn
];

@NotAShelf @Soliprem I need a second opinion, should cfg.lsp.clangdPackage and friends be added here, or should we remove them and let users configure via vim.lsp.servers?

since cfg.lsp.fqbn is required, I think we'll need to keep it regardless if the others get removed

@github-actions github-actions bot deleted the cmake branch October 1, 2025 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants