Skip to content
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

editor: initialize .vscode settings directory #4294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thoughtpolice
Copy link
Collaborator

People have brought this up before that we might want some vscode settings, so let's try adding a few basic ones?

These options are basically always useful in vscode; turn them on. Note that there is an editorconfig plugin for vscode and we do have a .editorconfig file but these options aren't set due to an old intellj-rust bug. Maybe we could get rid of it?

These are basically always useful in vscode; turn them on. Note that there is an
editorconfig plugin for vscode and we do have a `.editorconfig` file but these
options aren't set due to an old intellj-rust bug.

Signed-off-by: Austin Seipp <[email protected]>
@thoughtpolice
Copy link
Collaborator Author

This was split off from my Buck branch, but I figure it's a small and useful addition.

@PhilipMetzger
Copy link
Collaborator

PhilipMetzger commented Aug 16, 2024

I still don't particularly like that, we probably should get the .editorconfig working again.

@thoughtpolice
Copy link
Collaborator Author

thoughtpolice commented Aug 16, 2024

At the end of the day, I think if people are using other editors, we should encourage them submit patches to make the repo behave consistently too, rather than never letting anyone check anything in that improves a single editor. What's the issue? I suspect these settings are going to be fairly low maintenance, tbh. The amount of editors among common developers is probably what, 5 at max? It's just not worth worrying over. It's also relatively low risk since it isn't really impacting users and we don't need to worry about things like that.

With VSCode at least, we would probably want to make sure that the user is going to be recommended the editorconfig plugin to get these options — which still requires a settings.json file to properly suggest it (or maybe it's extensions.json?). I actually should add that ("suggested extensions" setting, whatever the name is), but at that point we've already gone 90% of the way so the inclusion of these options seems fine, to me, even if it's minor duplication, and it helps people who won't want to install plugins anyway.

Plus, editorconfig is, in practice, extremely limited. I like it for that reason, but ultimately, I think we might want to set other configuration options for an editor if we want it to behave better — for example to recognize specific filenames as specific languages, or set specific options for things like rust analyzer or clippy (which I do have in my buck branch too, but again I think that might happen eventually anyway.)

@PhilipMetzger
Copy link
Collaborator

At the end of the day, I think if people are using other editors, we should encourage them submit patches to make the repo behave consistently too, rather than never letting anyone check anything in that improves a single editor.

I think they already are, without needing some special configuration for their editor in the repo.

With VSCode at least, we would probably want to make sure that the user is going to be recommended the editorconfig plugin to get these options — which still requires a settings.json file to properly suggest it (or maybe it's extensions.json?)

Or we could take the approach of just providing documentation which plugins you'd need for each editor. And the actual users can chime in with suggestions and improvements, instead of just supporting a single one.

Plus, editorconfig is, in practice, extremely limited.

And that's a really good thing! Because if we start with VsCode/Codium then people will start to ask questions like why we don't provide it for Helix, Zed or the $FLAVOR_OF_THE_MONTH_EDITOR, because it clearly favors the VsCode/Codium users.

I think we might want to set other configuration options for an editor if we want it to behave better — for example to recognize specific filenames as specific languages, or set specific options for things like rust analyzer or clippy (which I do have in my buck branch too, but again I think that might happen eventually anyway.)

Yes, I just looked at the Buck2 PR where you set option to recognize Starlark files which only works for VsCode. OTOH, I see no issue with introducing a rust-analyzer.toml file, as it will work for everyone out-of-the-box. And the same for Clippy, if you can configure it in such a way.

@ilyagr
Copy link
Collaborator

ilyagr commented Aug 17, 2024

My problem with this is that it makes it difficult for me to have my own Workspace-level VS Code settings for the jj repo.

There's always the option of having settings.json.default file, similar to .envrc.default, but I'm not sure whether that's worth it for these settings.

@emilazy
Copy link
Collaborator

emilazy commented Aug 17, 2024

I agree that this would be annoying for that reason, and that a file of recommended settings is better than tracking the real thing. (There’s also some vague security worries, as with .envrc.)

@ilyagr
Copy link
Collaborator

ilyagr commented Aug 20, 2024

If we did want to have special treatment of VS Code, we could use https://code.visualstudio.com/docs/editor/extension-marketplace#_workspace-recommended-extensions to recommend the EditorConfig extension (assuming it works well) together with rust-analyzer. There could also be a .vscode/README.md explaining how to install a debugger (unfortunately, there are at least two different extensions depending on the platform and toolchain that you use) and linking to the JJ for VS Code extension (once that is available on Marketplace and if we are OK with advertising a potentially proprietary tool).

@PhilipMetzger
Copy link
Collaborator

Austin already suggested this, but that is not even better as it will mislead people which expect to work it out of the box. Those will then want to provide real options anyway. I think per editor docs will be better, but that is my opinion.

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.

4 participants