[WIP] Add "formatter" feature using "compiler" as a template #17145
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new ":formatter" command that mimics the behavior of ":compiler" by loading formatter scripts from the runtime directory. Key changes include adding new expansion and command definitions for formatter support, updating error messages, and extending command line completion to recognize formatter names.
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/vim.h | Added EXPAND_FORMATTER definition. |
| src/usercmd.c | Registered the new formatter keyword for command completion. |
| src/proto/ex_cmds2.pro | Declared the ex_formatter function prototype. |
| src/ex_docmd.c | Added macro definition for ex_formatter. |
| src/ex_cmds2.c | Implemented the ex_formatter command handling and script sourcing. |
| src/ex_cmds.h | Registered the new command with appropriate options. |
| src/ex_cmdidxs.h | Updated command indexes and count to include formatter. |
| src/errors.h | Added error message string for unsupported formatters. |
| src/cmdexpand.c | Handled formatter keyword in command line expansions. |
Files not reviewed (2)
- runtime/autoload/format.vim: Language not supported
- runtime/formatter/prettier.vim: Language not supported
|
Thanks. I would think this change makes a lot of sense. However let me mark it as draft for now. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
runtime/formatter/prettier.vim
Outdated
| endif | ||
| let current_formatter = "prettier" | ||
|
|
||
| FormatterSet formatprg=npx\ --yes\ prettier\ --stdin-filepath\ % |
There was a problem hiding this comment.
Like I commented in #16989 (comment), I don't think it's a good idea to use npx without the user explicitly choosing that. (If I understand what npx does correctly.)
There was a problem hiding this comment.
@dseomn In the JavaScript ecosystem, linters, compilers, formatters, etc. are generally not installed globally. Instead, they are treated as project dependencies and installed locally. Some of them, like Prettier, install an executable in node_modules/.bin/ but that directory is rarely, if ever, in people's $PATH, so the chances of the following to do anything useful are quite slim:
:[range]!prettier
Now, what could we do?
- Option number 1: figure out a path to the
prettierexecutable and call it directly. - Option number 2: figure out a path to
node_modules/.bin/and temporarily add it to$PATHbefore callingprettier. Something likePATH=/path/to/node_modules/.bin:$PATH prettier <args>. - Option number 3: use the ecosystem's standard way to execute a locally installed binary.
I went with option number 3, here, like I did for the "compilers" I added a few years ago because that is the most robust approach.
Possible issues with npx:
-
it might try to get stuff from the internet in some cases. If it can't it should be considered a fail and
&formatexprshould return 1.Or do you think the problem is elsewhere (security, privacy, bandwidth, etc.)?
The ideal (and, frankly, most common) scenario is:
- user is working in a project in which they already did a variant of
$ npm install, - Prettier is listed as dev dependency,
- Prettier is installed,
- user does
:formatter prettier, - user does
gggqG, which calls&formatexpr, which executesnpx prettier <args>, - the local
prettierexecutable is used.
Other less likely scenarios can be explored. The codefmt plugin linked to in the other thread has some logic for installing the required tools upon first invocation. Would you suggest we take inspiration from that?
- user is working in a project in which they already did a variant of
-
User doesn't have Node.js installed (
npxis part of Npm, which is part of Node.js), in which case&formatexprshould return 1.
There was a problem hiding this comment.
FWIW, the --yes argument could certainly be conditioned by a global variable, say:
let g:fmt_prettier_always_install = 1
but we would need to come up with a standardized naming convention so that other "formatters" could make use of it.
There was a problem hiding this comment.
In the JavaScript ecosystem, linters, compilers, formatters, etc. are generally not installed globally.
Interesting, I didn't know that. (I've almost never done JS dev, I was just coming to this because I'm interested in how vim uses formatters in general.)
Or do you think the problem is elsewhere (security, privacy, bandwidth, etc.)?
Mostly security, but also licensing issues.
For security, I don't think it's a good idea to run code that the user didn't explicitly choose to install. I've worked in multiple environments that had security policies about where software could be installed from or what software could be installed.
For licensing reasons, I don't think it's a good idea to install software in a way that the user didn't choose, since they might configure all the package managers they use to avoid specific licenses (e.g., AGPL). Then installing with another package manager that they didn't know was going to be used could install software with licenses that could cause trouble for them. (Maybe the repos that npx uses by default would prevent that for AGPL though? I'm not really familiar with that ecosystem.)
The ideal (and, frankly, most common) scenario is:
Does npx have a way to use a local binary if it's already installed, but fail if it's not? Would that make sense for typical use cases? Would it give a clear error message to help people install prettier if they want to?
There was a problem hiding this comment.
Interesting, I didn't know that. (I've almost never done JS dev, I was just coming to this because I'm interested in how vim uses formatters in general.)
Don't worry. We reinvent the wheel and the whole car every 3-5 years.
For security, I don't think it's a good idea to run code that the user didn't explicitly choose to install. I've worked in multiple environments that had security policies about where software could be installed from or what software could be installed.
IMO, doing :formatter prettier counts as implicit authorization, but I guess we could be more explicit…
- by prompting the user when
formatter/foo.vimis sourced, - or by doing that as part of
&formatexpr, - or by exposing some kind of
g:fmt_<formatter>_allow_install"option".
For licensing reasons, I don't think it's a good idea to install software in a way that the user didn't choose, since they might configure all the package managers they use to avoid specific licenses (e.g., AGPL). Then installing with another package manager that they didn't know was going to be used could install software with licenses that could cause trouble for them. (Maybe the repos that npx uses by default would prevent that for AGPL though? I'm not really familiar with that ecosystem.)
AFAIK Npm can't be configured to allow/disallow specific licences.
As mentioned above, I think that doing :formatter foobar (or :compiler barbaz, for that matter) IS explicit enough.
Maybe passing the following options to npx would alleviate your concerns:
--no, don't try to install the binary and just use the one that is already there, if there is,--offline, don't download stuff from the internet (or maybe--prefer-offlineas a middle ground?).
Like this:
FormatterSet formatprg=npx\ --no\ --offline\ prettier\ --stdin-filepath\ %
Scenario 1:
- user has installed the deps of the current project, including Prettier
npxuses the localprettier
Scenario 2:
- user has installed the deps of the current project but those don't include Prettier
- npx tries to get the package from the cache
- fails if the package has never been downloaded
- succeeds otherwise
- npx tries to get the package from the cache
Scenario 3:
- user is working in some random directory without
package.jsonornode_modules- same as scenario 2
There was a problem hiding this comment.
The codefmt plugin linked to in the other thread has some logic for installing the required tools upon first invocation. Would you suggest we take inspiration from that?
I was curious, so I looked at it. It looks like it uses npx with --no-install by default if npx is installed, but has a flag to configure that: https://github.com/google/vim-codefmt/blob/ff464a478202df40ae484e6e94a1d56587fcc69e/instant/flags.vim#L195-L206
And from https://docs.npmjs.com/cli/v8/commands/npx it looks like --no-install is a deprecated form of the --no you mentioned.
I also just realized that there's still a security issue when editing code in an untrusted repo, since it could have the directories that npx looks in. I'm not sure there's a good solution to that, but I think it's still worth avoiding installing things when a user doesn't expect it.
IMO, doing
:formatter prettiercounts as implicit authorization, but I guess we could be more explicit…
I would not expect that to authorize running any code that I didn't already install. I pretty often edit code in languages I'm not too familiar with. If the README says they use formatter foo and if foo is already installed or in Debian's repos I'm happy to use that, but I would not install a formatter from outside of Debian's repos. So I'd expect :formatter foo to fail if I don't have that formatter, and then I just would format by hand and hope that the repo I'm contributing to has CI to catch formatting errors.
Maybe passing the following options to
npxwould alleviate your concerns:* `--no`, don't try to install the binary and just use the one that is already there, if there is, * `--offline`, don't download stuff from the internet (or maybe `--prefer-offline` as a middle ground?).Like this:
FormatterSet formatprg=npx\ --no\ --offline\ prettier\ --stdin-filepath\ %
Why would --offline be needed if --no is already specified?
There was a problem hiding this comment.
Why would
--offlinebe needed if--nois already specified?
You are right, it is not.
I have made --yes optional, defaulting to --no.
| * ":formatter[!] {name}" | ||
| */ | ||
| void | ||
| ex_formatter(exarg_T *eap) |
There was a problem hiding this comment.
All of this logic could live in a plugin. ex_formatter could just call format#Formatter() ?
There was a problem hiding this comment.
I agree. @romainl can you explain why you think this should be an excmd instead of a builtin plugin?
There was a problem hiding this comment.
@justinmk @dvogel This proposal is entirely modeled after the "compiler" feature, which works through an internal ex_compiler command that sources the corresponding runtime "compiler" scripts:
| Compiler feature | Formatter feature | |
|---|---|---|
| Built-in Ex command | :compiler |
:formatter |
| Location of individual scripts | compiler/foo.vim |
formatter/foo.vim |
There was a problem hiding this comment.
I agree. @romainl can you explain why you think this should be an excmd instead of a builtin plugin?
I believe what @justinmk means is that the the Ex command :formatter should just call Vimscript function. So basically the command's declaration still lives in C, but the command's logic should live in Vimscript.
There was a problem hiding this comment.
I understood what he meant. I'm still waiting for a justification for diverging from established patterns, though.
This is a POC intended to illustrate the proposal I made in this thread.
In a nutshell:
:formattercommand similar to:compiler.$VIMRUNTIME/formatter/that define&formatprgand eventually&formatexprfor various formatters.:formatter <name-of-a-formatter>to set the related options properly, either manually or in a script.gq<motion>and so on.I have added a single "formatter" for now in order to make my point. Everything here certainly needs to be triple-checked and refined… and a real PR would probably include a bunch of other formatters, tests and so on.
Test case (
npmis required):Create a directory, change to it, and crate a JavaScript file:
$ mkdir foo $ cd foo $ touch index.jsPut the following content in the file:
Choose the
prettierformatter:Format the file: