Skip to content
This repository has been archived by the owner on Apr 1, 2020. It is now read-only.

[WIP] Allow the passing of a custom init.vim path. #784

Merged
merged 5 commits into from Oct 12, 2017

Conversation

CrossR
Copy link
Member

@CrossR CrossR commented Oct 11, 2017

This PR should allow the defining of a custom init.vim path in the Oni config file.

I need to check into some error checking, ie if an incorrect path is given, as well as some testing outside of Windows (Mac or Linux users would be useful, since I've only got Window + a few Linux VMs).

Also, is it acceptable to assume the user will sufficiently escape the path they provide?
Ie if they are on Windows and their init.vim is at "C:\Users\CrossR\init.vim", they would need to set it with escaped back slashes as follows:

"oni.customInitVimPath" : "C:\\Users\\CrossR\\init.vim".

Also since this is applied as an argument at startup, setting a custom path isn't applied until restarting oni currently. Is it worth looking into applying it straight away, or should that be a separate PR?

@CrossR
Copy link
Member Author

CrossR commented Oct 11, 2017

This could help with #436.

const customInitVimPath = configuration.getValue("oni.customInitVimPath")

if (customInitVimPath) {
return this.command(`e! ${customInitVimPath}`)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I should probably refactor this to just use the open command, so the logic isn't duplicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea 👍

@@ -53,7 +53,7 @@ export const registerBuiltInCommands = (commandManager: CommandManager, pluginMa
// Menu commands
new CallbackCommand("oni.config.openConfigJs", "Edit Oni Config", "Edit configuration file ('config.js') for Oni", () => openDefaultConfig(neovimInstance)),

new CallbackCommand("oni.config.openInitVim", "Edit Neovim Config", "Edit configuration file ('init.vim') for Neovim", () => neovimInstance.open("$MYVIMRC")),
new CallbackCommand("oni.config.openInitVim", "Edit Neovim Config", "Edit configuration file ('init.vim') for Neovim", () => neovimInstance.openInitVim()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that you factored this out to a separate method, thanks!

@bryphe
Copy link
Member

bryphe commented Oct 11, 2017

Cool, thanks for submitting this @CrossR ! I definitely think it makes sense to be able to override the init.vim path. There are some cases where users would prefer to have a completely separate init.vim between Oni/Neovim, so this helps accomplish that.

The only concern I have is that the configuration settings we have today are already confusing - oni.loadInitVim, oni.useDefaultConfig, and oni.customInitVimPath. I'd like to see if we could consolidate the oni.loadInitVim and oni.customInitVimPath configuration settings, if possible (since setting oni.customInitVimPath would imply oni.loadInitVim is true).

A couple of options I'm thinking about:

  • Change oni.loadInitVim to support string | boolean. If it is true, we would just load init.vim from its usual place. If it is a string, we'd load it from the custom path.
  • Only support custom paths - We could make oni.customInitVimPath be the way to using an init.vim, whether it is in the default place or not, and deprecate oni.loadInitVim. The downside is it would hose users that use oni.loadInitVim, we'd have to make it clear we are deprecating that value and how to upgrade - it would also be a bit more inconvenient for users who pick up their config in the default place, because they might have to look up where that is.

I'd lean towards the first one, just because it is simplest... but I'm sure there are other ways we could consolidate this and simplify the relationship between oni.loadInitVim/oni.customInitVimPath. Let me know what you think!

@CrossR
Copy link
Member Author

CrossR commented Oct 11, 2017

The only concern I have is that the configuration settings we have today are already confusing - oni.loadInitVim, oni.useDefaultConfig, and oni.customInitVimPath. I'd like to see if we could consolidate the oni.loadInitVim and oni.customInitVimPath configuration settings, if possible (since setting oni.customInitVimPath would imply oni.loadInitVim is true).

Yep, totally agree!
I did find myself thinking this as I was coding it, but wasn't too sure if it was the right way to go, though I hadn't considered letting oni.loadInitVim accept either a string or a boolean. That makes the logic a lot cleaner, and clears up the user config.

I'll have a look at implementing it now.

Also refactored the opening of the init.vim to use the existing open command.
@CrossR
Copy link
Member Author

CrossR commented Oct 11, 2017

Consolidated those two options together now and seems to work well!
Also swapped to using the built in open where I can, and updated some variable names to be more appropriate.

There is still some oddness with the interactions with oni.useDefaultConfig, but only the same that is in the live version, and this is outlined in the Wiki, so its possibly an issue to raise later.

@bryphe
Copy link
Member

bryphe commented Oct 12, 2017

Excellent, looks great! I just tried it and it works well - thanks for making the changes!

There is still some oddness with the interactions with oni.useDefaultConfig, but only the same that is in the live version, and this is outlined in the Wiki, so its possibly an issue to raise later.

Definitely, I'd like to see how to consolidate this at some point. But as you mentioned at least this change doesn't introduce any new issues, the problems are pre-existing 😄

I'll go ahead and bring this in. I made a quick edit to the wiki to detail that a string path can be accepted, but feel free to revise.

@bryphe bryphe merged commit 4bf40fd into onivim:master Oct 12, 2017
@CrossR CrossR mentioned this pull request Oct 12, 2017
@CrossR CrossR deleted the CrossR/436/InitVimLocation branch March 20, 2018 12:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants