Read key remappings from .vimrc #3908
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I would love to see this feature go live! Is anyone with permission to merge going to say anything about this? I think this is a easy solution for something that a lot of people want |
|
At this point, I figure I should rebase the branch and replace the |
|
@rdnlsmith Sorry the team is so backed up on PRs. I'm definitely interested in this - will skim now and revisit later. |
J-Fields
reviewed
Sep 5, 2019
| @@ -743,6 +752,7 @@ | |||
| "appdirectory": "0.1.0", | |||
| "diff-match-patch": "1.0.4", | |||
| "lodash": "4.17.15", | |||
| "fs": "0.0.1-security", | |||
There was a problem hiding this comment.
As you mentioned, vscode.workspace.fs seems to be preferred
J-Fields
reviewed
Sep 5, 2019
J-Fields
reviewed
Sep 5, 2019
J-Fields
reviewed
Sep 5, 2019
package.json
Outdated
| }, | ||
| "vim.vimrcPath": { | ||
| "type": "string", | ||
| "description": "Path to a Vim configuration file. If unset, it will check for $HOME/.vimrc or $HOME/_vimrc" |
There was a problem hiding this comment.
What about $XDG_CONFIG_HOME/nvim/init.vim? (https://neovim.io/doc/user/starting.html#vimrc)
Feel free to disagree, but my I think my preferred approach for this is have vim.vimrcPath default to an empty string. Then note here that if it's unset, we fall back on default locations (like $HOME/.vimrc, $HOME/_vimrc, and $XDG_CONFIG_HOME/nvim/init.vim) and then link to the part of the README which lists these locations.
A nice-to-have would be a pop up the first time it defaults to a location (which is probably immediately after you enable this feature) that says something like "We couldn't find your .vimrc. Click here to set the location" or "We found a .vimrc here: /path/to/.vimrc. Click here to use it"
There was a problem hiding this comment.
What about
$XDG_CONFIG_HOME/nvim/init.vim? (https://neovim.io/doc/user/starting.html#vimrc)
I skipped this initially because $XDG_CONFIG_HOME wasn't set by default on my Ubuntu machine. But, I can certainly add it, plus the default paths from your link.
Feel free to disagree, but my I think my preferred approach for this is have
vim.vimrcPathdefault to an empty string. Then note here that if it's unset, we fall back on default locations (like$HOME/.vimrc,$HOME/_vimrc, and$XDG_CONFIG_HOME/nvim/init.vim) and then link to the part of theREADMEwhich lists these locations.
I'm not sure I understand how this is different from what I did?
A nice-to-have would be a pop up the first time it defaults to a location (which is probably immediately after you enable this feature) that says something like "We couldn't find your .vimrc. Click here to set the location" or "We found a .vimrc here: /path/to/.vimrc. Click here to use it"
I like this idea! I'll see what I can do.
There was a problem hiding this comment.
I'm not sure I understand how this is different from what I did?
Sorry about that - I think we should support more default locations and list them externally (with a link in the description).
J-Fields
reviewed
Sep 5, 2019
J-Fields
reviewed
Sep 5, 2019
J-Fields
reviewed
Sep 5, 2019
|
@J-Fields Do you prefer |
Up to you |
|
|
jpoon
reviewed
Sep 7, 2019
jpoon
reviewed
Sep 7, 2019
jpoon
reviewed
Sep 7, 2019
jpoon
reviewed
Sep 7, 2019
src/configuration/vimrc.ts
Outdated
|
|
||
| // Don't override a mapping present in settings.json; those are more specific to VSCodeVim. | ||
| if (!collection.some(r => _.isEqual(r.before, mapping!.keyRemapping.before))) { | ||
| collection.push(mapping.keyRemapping); |
There was a problem hiding this comment.
What if duplicates are added? What if there is a malformed remapping -- can we have these settings undergo src/configuration/validators/remappingValidator.ts?
There was a problem hiding this comment.
If things go wrong, how do users debug this?
There was a problem hiding this comment.
Actually, I don't think it's possible that a bad remapping could be generated, based on what's currently being validated in remappingValidator. That said, remappings from .vimrc are added to the config before the config is validated.
jpoon
reviewed
Sep 7, 2019
| for (const line of lines) { | ||
| VimrcImpl.tryKeyRemapping(configuration, line); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we save the updated vscodevim configuration? Otherwise, we'll need to do this re-syncing every time vscode is launched.
If i update my vimrc, I'll need to re-launch vscode? Is that what most users will expect? Changes to the vscodevim settings on the otherhand don't require a restart.
There was a problem hiding this comment.
We could re-evaluate the vimrc if the user saves it from VSCode. We could also have a "source" command to manually re-evaluate
|
@rdnlsmith it doesn't matter. we'll squash when we merge the PR :) |
|
Let us know when the PR is ready for review again. |
Read key remapping commands from $HOME/.vimrc, $HOME/_vimrc, or a user-specified Vim configuration file. For each, build an IKeyRemapping object and append it to the appropriate collection, _if_ doing so will not override a remapping specified in the VS Code settings. Partially addresses #463. This implementation borrows heavily from Sheepolution/vimrc-to-json.
|
@jpoon I've fixed most of the issues that were brought up. Another round of review would be great when you get the chance. |
jpoon
approved these changes
Nov 4, 2019
| import { vimrcKeyRemappingBuilder } from '../../src/configuration/vimrcKeyRemappingBuilder'; | ||
|
|
||
| suite('VimrcKeyRemappingBuilder', () => { | ||
| test('Build IKeyRemapping objects from .vimrc lines', () => { |
There was a problem hiding this comment.
maybe a test case with a multi-line vimrc? and with other options that are ignored?
|
@J-Fields Hey, I'm sorry I never got back to this—it's been a busy month+ and time got away from me. I'm glad to see that you're fixing it up! |
|
No worries @rdnlsmith, thanks for getting the ball rolling on this feature! |
|
README.md currently says:
With this change we can remove this right? And perhaps add a small section on how to let it read your .vimrc file. |
|
@Sheepolution Yeah, there are still a few small changes I need to make, thanks for pointing that out. |
Travis tests have failedHey @rdnlsmith, Node.js: 8if [[ $(git diff-index HEAD -- *.js *.ts *.md) ]]; then git diff; echo "Prettier Failed. Run `gulp forceprettier` and commit changes to resolve."; exit 1; finpm testTravisBuddy Request Identifier: eec554a0-0353-11ea-86a0-93c38a831c21 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
Reads key remapping commands from $HOME/.vimrc, $HOME/_vimrc, or a user-specified Vim configuration file. For each, builds an IKeyRemapping object and appends it to the appropriate collection, if doing so will not override a remapping specified in the VS Code settings.
Which issue(s) this PR fixes
Partially addresses #463.
Special notes for your reviewer:
This implementation borrows heavily from Sheepolution/vimrc-to-json.
I haven't done much with node or TypeScript before this, so apologies in advance if I've made any "obvious" mistakes.