Propagate air.toml settings to client #160
Conversation
crates/lsp/src/notifications.rs
Outdated
| use crate::{main_loop::LspState, workspaces::WorkspaceSettings}; | ||
|
|
||
| #[derive(serde::Serialize, serde::Deserialize)] | ||
| struct SettingsNotifications { |
There was a problem hiding this comment.
| struct SettingsNotifications { | |
| struct DidChangeFileSettings { |
I would love if we could use names that feel like "real" LSP notifications
There was a problem hiding this comment.
Hmm... But we send this notification in did_open as well, not necessarily when a toml changed.
How about air/syncFileSettings? It seems like we do need a new prefix here?
There was a problem hiding this comment.
We agreed air/syncFileSettings is great 👍
editors/code/src/settings.ts
Outdated
| if (file.startsWith("file:///")) { | ||
| file = url.fileURLToPath(file); | ||
| } | ||
| return path.normalize(file); |
There was a problem hiding this comment.
Something doesn't seem quite right with indent-width. If I add a air.toml to dplyr and add indent-width = 4 and switch back to src.R then I see this
There was a problem hiding this comment.
If you grep for export class ChangeIndentationSizeAction extends EditorAction { in the positron source, you'll see that it seems like it updates both indentSize and tabSize to keep things in sync!
There was a problem hiding this comment.
Maybe if indent_style === "space" we also set tabSize? That keeps them in sync, but still allows for user customizable tab size when indent_style = "tab"
Interestingly if you have a tabSize = 4 but indentSize = 2 and actually format with tabs, VS Code does show you a little vertical line at the 2 space marker, even though the tab has a visual size of 4 (i.e. it visually shows you the info for indentSize)
There was a problem hiding this comment.
I wondered what's the right thing to do here. If we keep them in sync, we're working against an explicit VS Code feature, namely that you can set tabSize to be different than indentSize when insertSpaces is true, so that you can use tabs to format tables. You could imagine tabSize be set in a settings.json file. And the tables would be protected by air: ignore comments.
We can just not support this. According to threads in the vscode repo it's mostly needed for old corporate codebases. I'd be surprised to see R users miss this feature.
And then we avoid the strange mismatch in the status bar that I agree is confusing.
There was a problem hiding this comment.
I'm happy to not support this edge case
lionel-
force-pushed
the
feature/air-toml-propagation
branch
from
74ef0bc to
1262215
Compare
There was a problem hiding this comment.
Check out #173 first and see if you agree. I've been keeping the serde derives minimal so far with these types.
There was a problem hiding this comment.
Really loving everything in one file!
I don't disagree but I can't promise they won't creep back in over the course of a PR that changes direction in terms of serde requirements... I don't think such derivations on non-generated types bloat the binary that much? I agree that it's nice to avoid implying a type crosses an interop boundary though. |
Co-authored-by: Davis Vaughan <[email protected]>
Co-authored-by: Davis Vaughan <[email protected]>
lionel-
force-pushed
the
feature/air-toml-propagation
branch
from
de0d6d2 to
884f555
Compare
Closes #160.
Controlled by
air.settingsBackpropagation. Uses the same approach as the editorconfig extension: when the active editor changes we apply the air.toml settings to the active editor.We send custom
tomlSettingsLSP notifications to the client:The client maintains a map of settings. Ideally we wouldn't have to do that but in practice we need to because invisible editors can't have their settings changed. So instead of changing the settings when the notification arrives, we change it when the active editor changes.
Unlike editorconfig, we change
indentSizerather thantabSize. This is for consistency with the principle thattabSizeis a user preference setting. Note that it seems some VS Code feature such as move line up are not affected byindentSizeand instead usetabSize. There might be other bugs of this kind, which could explain why editorconfig decided to changetabSize.