vim: Add :RustRun and associated commands #14480
Conversation
|
Naming the commands It's customary for Vim plugins to prefix all of their commands with a custom prefix. Or just use uncommon command names. Please don't exacerbate the problem of Vim plugins colliding. We have enough of that as it is in the Vim community. |
|
@Valloric They're buffer-local commands that are only defined when the buffer has the |
|
I have some FUD about :Run executing arbitrary code. Not sure that it shouldn't be a thing, but I'm wonder if it should have to be explicitly bound up in the vimrc and just exposed as a function by default. With that said, I would love if the compile step could be decoupled, and hooked up to the |
|
@kballard I've noticed that, and while it makes the problem less of an issue than otherwise, it's still a bad idea to keep the names. Most plugins aren't filetype-specific so will collide with these names if they use In general, unprefixed commands are left for the user to create/map in their vimrc. Again, this is basic Vim plugin hygiene. |
|
@richo Not sure what you mean about FUD. The whole point is to provide something useful. As for |
|
@Valloric Any non-buffer-local, non-filetype-specific plugin that uses the name
That's a reasonable argument. But what about mappings? You can't prefix those. It seems mildly odd to require prefixes on commands when mappings tend to be more common and more likely to conflict.
Is there any authoritative source for this, or just your personal opinion/experience? I certainly agree with you that commands should be prefixed if they are global, but I'm not convinced that's necessary for buffer-local filteype-specific commands. As an example of precedent, Vim's built-in |
|
Here's an online source for Note that this is by Tim Pope, which seems pretty authoritative to me. |
|
Sorry, that was very much two different points. I'm not sure I agree with you about barrier to entry. It just means that instead of saying: You an use instead it should read: if you put I'm not convinced that's too much extra hassle. the |
|
@richo Telling people that they have to put some arcane |
Agreed 100%, but they certainly do exist. I'm actually far more worried about this conflicting with a user's custom command.
And that's why plugins shouldn't be creating default mappings unless they are critical to the functioning of the plugin and even then, there must be a way for the user to customize the mapping the plugin creates. You haven't allowed that in your plugin; it's
It's less necessary, but even such buffer-local commands conflict with global commands. I can guarantee you without a doubt that there's a non-trivial number of users out there with
Tim Pope is great, but IMO that was a bad call. Is renaming |
| let &foldenable = foldenable | ||
| endfunction | ||
|
|
||
| function! <SID>Jump_Back() |
There was a problem hiding this comment.
Why <SID>? Just prefix with s:. Same below.
There was a problem hiding this comment.
Good point. This was a copy & paste, but it's certainly worth fixing.
It reduces the usability of the command. It's not a huge issue, but I'd rather produce something that's usable and matches existing precedent, then reduce usability for the sake of an overabundance of caution. If a user has a global command named Do you have any strong precedent you can cite for using prefixed command names in filetype plugins? |
Vim commands are far better off being verbose and descriptive; if they're something the user wants to use often, they can trivially create a mapping: This is the approach YCM uses (it even recommends custom mappings for commands) and I've never had a single complaint about YCM command name lengths being so long that they hurt usability. And the names are really long; one of the most frequently used YCM commands is
So it will silently override the user's command. That's even worse; the problem with mappings silently overriding other mappings by default is the reason why we got In other words, you want to tell the user about a conflict so that they can resolve it instead of having something silently override.
I don't, but your argument boils down to "Others have made this mistake before so why can't I make it too?" I've lost track of the number of hacks I've had to add to YCM because other plugin authors haven't been as diligent as they should have. |
In fact, I do have it. YCM creates several commands that could easily be filetype-specific (like All of these commands have a |
|
Oh, and here's another benefit of using a consistent prefix for all of your commands: typing The prefix increases usability. |
|
@Valloric You're citing as precedent a plugin (which I've never used, BTW) that appears to provide global behavior. I don't see how that's an appropriate precedent for filetype-specific buffer-local behavior. I appreciate that you have experience writing Vim plugins, but I just don't think your objections are particularly relevant to this case. It would make a lot of sense if this was a global command, or at least affected more than just one specific brand new filetype (e.g. if it were modifying an existing filetype then it would need to take more care not to step on the toes of anything else related to that filetype). I would really like to see some sort of valid precedent here for what you're arguing for. I've already shown precedent from Tim Pope, in source distributed with Vim itself, for doing exactly what I'm doing here. |
Some commands only work in some filetypes (and are documented as such). Other functionality is global. Either way, I've exhausted all of my arguments along with my energy for further discussion. I don't see us convincing each other so we'll agree to disagree and call it a day. :) |
|
@richo I just experimented with adding the output of |
|
@Valloric My basic point is that the Rust plugin "owns" the That being said, the one argument you made about typing Is there any good solution for this? For telling the user how to use a command without having them go back to the source (which is where they'd learn about the filetype commands under normal circumstances)? If you have a good answer for this, then that may be worth changing the names for. |
This still ignores the fact that if a global
Step 1 is to discover that command
Use cases:
|
|
|
I think I would generally prefer |
|
Oh, by the way: see also #7787. |
|
@Valloric @chris-morgan Ok, I renamed the commands and wrote a full documentation file on it. |
Thank you! Please also make sure that |
|
@Valloric |
|
Added |
lilyball
changed the title
lilyball
changed the title
|
At this point I'm satisfied with what I have here. r? |
|
FWIW, lgtm. |
|
|
||
| function! s:Run(path, bang, rustc_args, args) | ||
| try | ||
| let exepath = tempname() |
There was a problem hiding this comment.
For Windows, I believe this will need to have .exe appended.
Define a command :Run to compile and run the current file. This supports unnamed buffers (by writing to a temporary file). See the comment above the command definition for notes on usage. Define <D-r> and <D-R> mappings for :Run to make it easier to invoke in MacVim. Define a command :Expand to display the --pretty expanded output for the current file. This can be configured to use different pretty types. See the comment above the command definition for notes on usage. Create an autoload file and put function definitions there to speed up load time.
|
@chris-morgan Both issues are now fixed |
| COMMANDS *rust-commands* | ||
|
|
||
| :RustRun [args] *:RustRun* | ||
| :RustRun! [rustc-args] [--] [args] |
There was a problem hiding this comment.
Why not always allow a --? I know that all that is said about the bang at the end of an Ex command is that it “makes the command behave in a different way” (:_! help), but this case doesn’t feel right to me; the bang’s existence or non-existence is merely trivial sugar.
I’d just as soon see it unified:
:RustRun [[rustc-args] --] [args]
Which would allow
:RustRun args
:RustRun -- args
:RustRun rustc-args --
:RustRun rustc-args -- args
These are achievable in the current behaviour thus:
:RustRun args
:RustRun args
:RustRun! rustc-args
:RustRun! rustc-args -- args
I think the precedent in bang behaviour is more along the lines of whether it should :update or not.
There was a problem hiding this comment.
I suspect that it's more common to want to pass flags to rustc than to the generated binary. I'd rather type :RustRun! -O than :RustRun -O --. To that end, if I were to get rid of the bang, I'd have :RustRun args be equivalent to the current :RustRun! args and require :RustRun -- args to pass args to the binary. But I also thought that was a bit nonintuitive. For someone who knows the :RustRun command is there but hasn't read the docs in detail will likely assume that argument are passed to the binary.
FWIW, Syntastic has recently removed its Rust checker |
|
@japaric Really? That seems weird. Even if it only detects some errors, it's still better to have it at all than not have it. |
…ust-lang#14480) This extra condition prevents a problem when removing the '}' in: ```rust ( // There was an opening bracket after the parenthesis, which has been removed // This is a comment }) ``` Removing the whitespaces, including the linefeed, before the '}', would put the closing parenthesis at the end of the `// This is a comment` line, which would make it part of the comment as well. In this case, it is best to keep the span on the '}' alone. changelog: none Note to reviewer: `manual_inspect` and `collapsible_if` were two lints exhibiting this problem in cooked up test cases, which have been included as non-regression tests.
Define new vim commands
:RustRun,:RustExpand,:RustEmitIr, and:RustEmitAsm, including full documentation on these commands and all the options.