Skip to content

Format vscode extention and LSP files consistently #748

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

Merged
merged 3 commits into from
Jul 3, 2025
Merged

Conversation

lionel-
Copy link
Collaborator

@lionel- lionel- commented Jul 2, 2025

The TS files in the LSP and VS Code extension are currently inconsistently indented, sometimes with spaces, sometimes with tabs. As I work in Zed, an editor that doesn't have automatic detection of indentation, it's very inconvenient for me to write code in these files.

So I've reformatted all files using VS Code's TS formatter (which I can also use from Zed) thanks to this great extension: https://marketplace.visualstudio.com/items?itemName=lacroixdavid1.vscode-format-context-menu. The internal formatter is limited but I think this is an advantage as it's mostly inconsistent whitespace that has been changed in this PR. Reformatting with Biome or Prettier would have far more impact and so likely require a little more thought and discussion.

I know it's always a bummer and a potential source of conflicts to resort to a large commit that sprinkles the repo of trivia changes, but I think this will significantly improve workflow as we contribute to the Quarto extension.

Another change in this PR is that Zed gains a project settings file, and I've disabled automatic detection of indentation in VS Code settings so that it always use the configured indentation style and we don't fall back into the inconsistent state. Both IDEs are now configured to use spaces. I've checked that this was the main indentation style in apps/lsp and apps/vscode.

@lionel- lionel- requested review from cscheid and DavisVaughan July 2, 2025 12:39
@cscheid
Copy link
Contributor

cscheid commented Jul 2, 2025

Thanks, but I honestly don't know if I'm comfortable merging this until we agree to be using the same TS formatter everywhere in Posit. For example, there will be a mix of deno fmt and the output of this, and I don't know if they agree.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 2, 2025

@cscheid Most people I know are using VS Code's language server for TS development, and it's possible to use it in other editors. We do have it enabled by default, see:

"editor.defaultFormatter": "vscode.typescript-language-features",
"editor.formatOnSave": true

The main issue fixed here is the inconsistent indentation style. A potential reason we ended up in this situation is the combination of having copied over files from other projects, and VS Code respecting the document's indentation instead of the project setting. This is what this PR fixes.

@cscheid
Copy link
Contributor

cscheid commented Jul 2, 2025

combination of having copied over files from other projects

This is what this PR fixes.

I understand that. You've identified the outcome; I just want to make sure we understand the cause. I'm not positive that copying over files was the entire reason we arrived here.

There might be two LSPs potentially at play: the TS one you're using, and the one from Deno. We certainly use the latter in quarto-cli, and might use in some files here because Quarto's VSC extension can turn on the Deno extension for its files at times.

All I want is to make sure we're not in a situation where we're ping-ponging the preferred indentation.

@lionel-
Copy link
Collaborator Author

lionel- commented Jul 2, 2025

Gotcha I'll take a look at Deno and make sure that it's configured to use 2 spaces for the apps/ projects formatted in this PR.

I don't mind switching to Deno for formatting these sub projects but that could be the subject of an ulterior PR. This one is mostly whitespace.

By the way I learned from @DavisVaughan today that we can add a file like this to exclude style-change commits from blame (we need the commit hash so that would have to be added later): https://github.com/biomejs/biome/blob/main/.git-blame-ignore-revs

@cscheid
Copy link
Contributor

cscheid commented Jul 2, 2025

I think we're good to merge this.

I reviewed all the files in the PR and in the monorepo as well, and I think we've succesfully contained the Deno LSP to quarto-cli (and that's a good thing. We shouldn't let that escape containment.)

@lionel- lionel- merged commit a1513ef into main Jul 3, 2025
1 check passed
@lionel- lionel- deleted the task/format branch July 3, 2025 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants