Recomand dts lsp

[kylebonnici]

PR check-list

• Branch has a clean commit history
• Additional tests are included, if changing behaviors/core code that is testable.
• Proper Copyright + License headers added to applicable files (Generally, we stick to "The ZMK Contributors" for copyrights to help avoid churn when files get edited)
• Pre-commit used to check formatting of files, commit messages, etc.
• Includes any necessary documentation changes.

CC: @urob should I set the defaults to insert space and tab size 4? or should this repo follow https://docs.zephyrproject.org/latest/contribute/style/devicetree.html ?

[urob]

I haven't used the LSP with my editor yet but I have tested it with the related dts-linter to auto-format the code base. The upcoming 0.51 release should be able to handle the entire ZMK base well with two exceptions:

• manually aligned matrices require guarding with dts-fmt off to preserve the formatting
• in one instance we have to remove a space in MACRO (ARGS) to work around the limitation in kylebonnici/dts-lsp@0254faa

Re: formatter settings: I think useSpaces with tabWidth=2 is closest to the existing codebase

FWIW: I also have a WIP pre-committer action that we could set up eventually: https://github.com/urob/mirrors-dts-linter (I first planned to PR it to dts-linter, but apparently pre-commit doesn't allow pulling from npm for packages that share the repo name)

[kylebonnici]

I can adjust the defaults for LSP
In the settings.json that is not an issue

What I wanted to figure out is if divergence form https://docs.zephyrproject.org/latest/contribute/style/devicetree.html was intentional.

[nmunnich]

First of all, thank you very much for your work on the dts formatter. I'm quite excited about where this can go.

I think before we can set it as a recommended extension, we probably would need the dts formatter to be run on all the files in the repo, and some documentation page describing setup and usage, especially for more end-users. That would require some method of handling the keymaps since they are often custom formatted. @urob had the idea of an exclusion block, there was some discussion on Discord, not sure if anything has happened there yet?

The other concern I have is one of a potential supply chain attack, 5+ years down the line, considering how VSCode handles extensions currently. I'm not sure if you planned to try and move this under the Zephyr project, or if we should pull it into ours/maintain a fork, or some other solution. Just a bit of discussion here would be nice to have.

Regarding your formatting question, I'm not certain but I think it is intentional that we diverge from Zephyr to conserve on horizontal space for long arrays. I would say adjust the settings to keep it as-is for now, and perhaps we will realign with Zephyr in the future with the help of the formatter.

[kylebonnici]

Re formatting settings noted will
Make change to pr

Re keymap, are these used like overlays or like dts (board files) or like dtsi files, asking to make changes to the LSP to consider these files in the list of languages and in any other relevant way.

Re supply chain attacks, I cannot see the project becoming part of zephyr given it is in typescript, maintaining a fork also does not solve the issue as the recommended extensions do not specify a version + then a new extension will need to be released with a different name. For CI having a lock file and updating only when needed can help limit the attack surface.

From my perspective I do my best to safeguard against supply chain attacks by

• constantly monitoring the npm audit,
• limiting the update to dependencies untill these are needed
• use latest recommended way to release to npm to avoid using tokens

These concerns for supply chain attacks was also discussed when the dts-linter discussed to be added to zephyrs CI I am happy to hunt down the PR so you can see what remains to be addressed.

[nmunnich]

Re keymap, these are essentially just overlay files that are labeled differently to show their dedicated purpose.
You can see how the custom formatting is done e.g:
https://github.com/zmkfirmware/zmk/blob/main/app/boards/shields/corne/corne.keymap
Or
https://github.com/zmkfirmware/zmk/blob/main/app/boards/shields/cradio/cradio.keymap

Re supply chain, I would very much appreciate being linked to that PR. If it's good enough for Zephyr it tends to be good enough for us, though we do have a greater number of "less techy" users so a bit of extra care is warranted when discussing anything they might reasonably encounter.

[kylebonnici]

This is the PR that introduced the linter to Zephyr zephyrproject-rtos/zephyr#92334

Possibly relevant comments from this PR:

• CI: devicetree: Add compliance and formatting check zephyrproject-rtos/zephyr#92334 (comment)
• CI: devicetree: Add compliance and formatting check zephyrproject-rtos/zephyr#92334 (comment)
• CI: devicetree: Add compliance and formatting check zephyrproject-rtos/zephyr#92334 (comment)
• CI: devicetree: Add compliance and formatting check zephyrproject-rtos/zephyr#92334 (comment)

By bundling in to one js and moving all dependencies to dev dependencies, npm i dts-linter (even if CI should use npm ci and have lock file) will be limited to 2 packages dts-lsp and dts-linter and the lockfiles these two npm packages use.

Also these project are release to npm using https://docs.npmjs.com/trusted-publishers

[kylebonnici]

I have actioned the changes to recommended setting to insert spaces and tab size 2....

However I have some reservations on this, If a board file in the ZMK imports a dtsi file from zephyr, and user navigates to this file formating will be different and LSP will complain that is is not formatted properly unless we also set formattingErrorAsDiagnostics: false to disable this.

[joelspadin]

IMO, showing formatting errors as diagnostics in VS Code for any language just results in distracting noise. If the result is auto formatted anyways, then there's no point in showing error squigglies under text that will just be fixed automatically by tools.

I haven't looked into how the dts linter is configured, but ideally it should use our configuration for files in ZMK and Zephyr's configuration for files in Zephyr. If that is not possible, then we should tell it to only check files that are part of ZMK, assuming it has allow/deny list options.

Regarding indent size, it looks to me like 4 spaces is most common? I checked a few files like https://github.com/zmkfirmware/zmk/blob/main/app/module/boards/nicekeyboards/nice_nano/nice_nano.dts, https://github.com/zmkfirmware/zmk/blob/main/app/boards/shields/corne/corne_left.overlay, https://github.com/zmkfirmware/zmk/blob/main/app/dts/common/arduino_uno_pro_micro_map.dtsi, etc. and while there are some exceptions using 8 spaces, I haven't found anything using two spaces yet.

I think the supply chain concern was also about the VS Code extension rather than the NPM package.

[kylebonnici]

IMO, showing formatting errors as diagnostics in VS Code for any language just results in distracting noise. If the result is auto formatted anyways, then there's no point in showing error squigglies under text that will just be fixed automatically by tools.

That is one view but a lot for formatters do this hence why it is configurable, the default value is something that can be discussed.

Regardless if squigglies are show saving that file (if auto formattin) is on will still effect change.

AFAIR for the below example this comes from prettier/prettier

then we should tell it to only check files that are part of ZMK, assuming it has allow/deny list options.

This is not something the LSP should support but rather the editor e.g. VS Code by providing workspace configuration that differ see https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_configuration. In vscode this should be doable using Multi-Root Workspace in vscode, will have to test this out but is see no reason I cannot make it work if it is not already working.

In that case I can amend the PR to add a vscode multi root workspace see https://code.visualstudio.com/docs/editing/workspaces/multi-root-workspaces

I think the supply chain concern was also about the VS Code extension rather than the NPM package.

I think it can apply to both, I am open to changes to the release process to accommodated this PR. What was the solution to mitigate this for all of the other extension in .vscode/extensions.json?

[caksoylar]

Regarding indent size, it looks to me like 4 spaces is most common?

In the documentation we try to stick to 4 spaces indent, no tabs. Would be great if that's the standard in the repo.

[kylebonnici]

Applied some changes to this PR to allow different formatting settings (needs kylebonnici/dts-lsp#154) @joelspadin.

User should use the workspace for this to work