vscode: do not autoformat markdown or toml files

[thoughtpolice]

Summary: This caught me several times and several instances of autoformatting have slipped by in some of my commits; generally this is a PITA when you're doing something like modifying markdown files and Visual Studio Code "helpfully" autoformats the entire document for a 1-byte change, creating a 100+ line diff out of thin air.

This is of course a function of the specified users autosave configuration and enabled extensions. I can't possibly account for everyone's integrations or workflows, but at least for me, I think, it's OK to turn off autoformatting here because we aren't enforcing a consistent one anyway.

I do think that adding autoformatting for some of these things may be a good idea. But in the mean time I think this is just a helpful safeguard.

As an example of this problem, I've included a small diff to remove a bunch of redundant whitespace from the top-level Cargo workspace file; this is exactly the kind of autoformatting that I think isn't good unless everyone agrees on it and it only serves to bloat diffs if someone changes it later (and no, there isn't really a standardized format for Cargo.toml yet from what I can tell — though, tellingly, the related style guides for Cargo.toml in the manual don't seem to recommend things like "add 60 characters of whitespace for no reason to align comments to column 80".)

Change-Id: Iea17b611d58e65135fdc702f7b59f46c

Checklist

If applicable:

• I have updated CHANGELOG.md
• I have updated the documentation (README.md, docs/, demos/)
• I have updated the config schema (src/config-schema.json)
• I have added tests to cover my changes

[arxanas]

Works for me 🤷

[joyously]

Does the project have a .editorconfig file?
See https://editorconfig.org/

[thoughtpolice]

Does the project have a .editorconfig file? See https://editorconfig.org/

I actually checked .editorconfig first before anything else but, alas, it has no keys or options for controlling things like "format this file on save." It's mostly just basic character encoding and whitespace options, still.

(While I like .editorconfig a bunch, I think part of its appeal is that it's simple and very easy to semantically understand. On the other hand I think that means it's hard to evolve, and so it's pretty much "done" and so some of these things like "format on save" are things it missed out on specifying.)

[ilyagr]

I'm not a huge fan of this: if somebody set up autoformatting on save that actually works for them, why shouldn't it work in our repo? If somebody set up autoformatting on save that doesn't work for them, why should we prevent them from learning that they don't like their config?

Either way, since this config doesn't make autoformatting work perfectly for our repo, I don't think it's worth making somebody's editor behave differently in our repo compared to another repo. My worry is that this will cause as much confusion as benefit. But this might be a wrong prediction and it is not a hill I'd die on; if this really makes people's lives more convenient, I'm OK with it.

For TOML, formatting-on-save seems less important, so my feelings are less strong either way.

As an aside, we'd benefit from a good Markdown auto-formatter; I'd rather standardize on some plugin (is "Prettier" good? I think it would work in other editors), set up a presubmit check, and use .vscode to recommend it and set up a config. That'd require some research in whether we actually like its formatting, whether it's a reasonable thing to install in editors other than VS Code, and how to set up a presubmit check.

BTW, I set up formatting on save for rs files only (mainly because it matches the presubmit check). For Markdown, I use the "Rewrap" plugin currently, manually. It doesn't do the entire job but seems good at what it does do.

[thoughtpolice]

I'm not a huge fan of this: if somebody set up autoformatting on save that actually works for them, why shouldn't it work in our repo? If somebody set up autoformatting on save that doesn't work for them, why should we prevent them from learning that they don't like their config?

Well, the same reason it happened to me: because it makes patches more annoying to write! If the autoformatter accidentally causes something to be committed, then undoing it is often an annoying dance of rigamarole, to either turn off those features, use another editor, etc.

But also, I do want those autoformatters on my own projects. I just don't think they're appropriate here since, in lieu of a chosen style, it mostly will make patches more annoying to review. So I guess you could make an argument I should add my own .vscode files to my own projects there and use that instead (turn it on), rather than this patch (turn it off.)

As an aside, we'd benefit from a good Markdown auto-formatter; I'd rather standardize on some plugin (is "Prettier" good? I think it would work in other editors), set up a presubmit check, and use .vscode to recommend it and set up a config. That'd require some research in whether we actually like its formatting, whether it's a reasonable thing to install in editors other than VS Code, and how to set up a presubmit check.

I'd also be really happy with this, but I'll be honest, part of the reason I submitted this instead of that is because formatting discussions can get into wonderfully long bikesheds. :) And I'm not sure that, unlike Rust or Go, there are clear standards in this area.

But I actually would be 100% happy to have some standards here, if it looks like we can get something working for everyone.

BTW, I set up formatting on save for rs files only (mainly because it matches the presubmit check). For Markdown, I use the "Rewrap" plugin currently, which doesn't do the entire job but seems good at what it does do.

Yeah, I also use rewrap actually. The Alt+Q is fantastic, though now that you say this, I'm wondering why my autowrapping settings are so aggressive, because it doesn't seem like this should be the default? I will have to do some spelunking on my config perhaps. I wonder if it's specifically some trigger or some global setting...

[martinvonz]

I'd also be really happy with this, but I'll be honest, part of the reason I submitted this instead of that is because formatting discussions can get into wonderfully long bikesheds. :)

I personally care much more that we have some standardized formatting than exactly what it looks like.

[thoughtpolice]

I'd also be really happy with this, but I'll be honest, part of the reason I submitted this instead of that is because formatting discussions can get into wonderfully long bikesheds. :)

I personally care much more that we have some standardized formatting than exactly what it looks like.

I agree. I think the major issue is probably we don't want to tie someone to vscode, right? I haven't taken a poll, but I'd be little surprised if literally 100% of the active devs used it? (Or maybe we do, thanks to rust-analyzer being good.) I wonder what options we have available for this that most of us can use and easily acquire... I might look around for that.

[martinvonz]

Ah, I thought you meant bikeshedding the details of how to format. I agree that don't require a particular editor.

[ilyagr]

I looked into Markdown formatting a bit. I might try formatting the jj repo with the two options I found when I get a moment.

I also don't want to create any barriers for first-time contributors that want to help us with documentation. Unfortunately, there seems to be no option that's trivial to install in the Rust ecosystem.

So, I think any presubmit checks for Markdown formatting would have to be optional.

Prettier

Prettier seems to be pretty bad, at a glance. It converts *blah* to _blah_, which I find annoying. Less definitely but perhaps more importantly, Markdown support seems to be an afterthought for them, it's more of a HTML/CSS/JS/etc tool. See also prettier/prettier#11828 for problems people have with it.

As a pro, it's very well integrated in different editors. It can also be run with https://pre-commit.com/ (see below).

Mdformat

There's https://github.com/executablebooks/mdformat. (This is different than an internal Google tool of the same name). It claims to be better than Prettier, but doesn't have any editor integrations I could find. It only integrates with https://pre-commit.com/.

mdformat also makes no effort to be easy to use outside the Python ecosystem. https://pre-commit.com/ seems to be a little better about that, and claims to be able to install mdformat.

Since jj doesn't support pre-commit hooks, we'd probably have to run it with https://pre-commit.com/ manually (pre-commit run). https://pre-commit.com/ does have some editor integrations, but they seemed a little sketchy.

Aside on pre-commit hooks

I'm not sure whether pre-commit hooks designed for git are a good idea for jj, as they might be too slow. They budget latency for a tool that's run on explicit user action, but jj would run them on every save. A command to run pre-commit run with something like jj run would probably be better.

After writing this section, I realized there's #405 tracking this. I think it came to a similar conclusion.

Update: Markdownlint

There's a third option I haven't really explored, "markdownlint". It's a library and has a VS Code plugin. I'm not sure which CLI is best for use with pre-commit or Github actions: https://github.com/igorshubovych/markdownlint-cli or https://github.com/DavidAnson/markdownlint-cli2, probably the latter.

markdownlint has a whole bunch of options.

Aside on Chromium

FWIW, the Chromium project seems to have given up on formatting markdown.

[ilyagr]

You can take a look at my first attempt at auto-formatting: ilyagr@mdformat and ilyagr@prettier.

Clearly, we'd need to exclude some files from overly aggressive formatting (Update: Actually, changing some options helped a bit here), but this can give a taste of what it looks like. Also, you can try the pre-commit run --all-files workflow and see if it works for you.

Prettier also formats a bunch of non-md files. Pre-commit should be able to prevent that, I think.

Prettier actually does pretty well. I'm worried a bit about prettier/prettier#11828, but it's theoretical for now. I guess I'd have to live with _italics like these_ if we choose it for its editor support.

OTOH, we can also add a TOML formatter to the pre-commit hooks.

Fortunately, neither of the tools has many options, so there's a limited space for arguing about the style if we use either of them.

Update 2: I updated the configs to make them wrap text in ~similar way. Another thing that annoys me with prettier is that it changes code blocks defined with

```
(code)
```

into codeblocks defined with 4 spaces. Generally, I prefer the former.

[ilyagr]

I found another interesting Markdown formatter: https://github.com/dprint/dprint. It's used by the comprehensive-rust course: google/comprehensive-rust@aaca44f. Apparently, it can both format Markdown and call cargo fmt.

[thoughtpolice]

While working on #2273 I was once again bitten by vscode "helpfully" doing autoformatting due to my settings using it by default, so to stop the diff from bloating, I had to snip out the relevant parts, put it in another file, jj restore the original, then insert the changes back into the file. Rather annoying.

Regardless of what we choose, I realize I'm not sure what kind of editor integration it will end up having, so I'd still like to see this merged. For example dprint looks good, but if there's no vscode support, I'd prefer if we used it as a CI check and disabled the users editor integration. Otherwise your editor is basically guaranteed to break the standard. If vscode support came along later, we could change the preferences to use it for formatting those files.

[martinvonz]

LGTM. I only skimmed the Conversion tab but I didn't see any objections from anyone else either.

[martinvonz]

While working on #2273 I was once again bitten by vscode "helpfully" doing autoformatting due to my settings using it by default, so to stop the diff from bloating, I had to snip out the relevant parts, put it in another file, jj restore the original, then insert the changes back into the file. Rather annoying.

It might have worked to use jj diffedit to remove the unwanted changes.

[ilyagr]

I'm not very happy about this. For one, this is incompatible with somebody having another per-workspace config for their jj repo.

I wonder whether VSCode provides some sort of "suggested config", so this'd work similarly to the envrc.recommended.

[ilyagr]

I'm not sure, BTW, whether this problem is theoretical or practical: do people often use .vscode/settings.json for something else? So, if this matters to you enough (and the sort-of-solution from my other comment doesn't work), we could still go with it.

[ilyagr]

My preferred (but imperfect) solution to such a usecase would be to have you put .vscode into a global gitignore or .git/exclude and have your own vscode config.

On problem I see with this is that finding the per-repo .git/exclude with jj is tricky with non-colocated repos (I'm not even sure it's supported).

A version of this idea (which is currently hard to do) would be to provide a script that creates that setup.

[thoughtpolice]

Closing this for now, as there's division on this and it can be worked around locally for me in various ways.