Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow A Key/Value Interface #412

Open
starcraftman opened this issue Mar 23, 2014 · 9 comments
Open

Allow A Key/Value Interface #412

starcraftman opened this issue Mar 23, 2014 · 9 comments

Comments

@starcraftman
Copy link
Contributor

Raised the initial issue in #403. I guess we can discuss that change here.

Gmarik suggested some key/value system for the Plugin command so that instead of json like dict we did something like;

Plugin 'somePlugin' name=newName, rtp=vim/

Thoughts? @gmarik @jdevera

As a side note, I made a rudimentary pass previously on plugin, now on this branch:
https://github.com/starcraftman/vundle/tree/key_value

Basically, I just searched for some key/value indicator (in this example '=') then packed them in dict. Strings without the = get passed on as is.

@gmarik
Copy link
Contributor

gmarik commented Mar 23, 2014

Your config#plugin implementationis missing rtp manipulation(unlike config#bundle counterpart)
Therefore my guess is that you're also thinking about improving startup speed, but then we need something like PluginInit.

Otherwise i'm confused, and can't get this branch to work in my tests…

I think this is what's missing:

diff --git a/README.md b/README.md
index 95c5ec0..9154e13 100644
--- a/README.md
+++ b/README.md
@@ -71,6 +71,8 @@
    Plugin 'file:///home/gmarik/path/to/plugin'
    " ...

+   PluginInit " initialize plugins
+
    filetype plugin indent on     " required
    " To ignore plugin indent changes, instead use:
    "filetype plugin on
diff --git a/autoload/vundle.vim b/autoload/vundle.vim
index 3256d2c..ef07090 100644
--- a/autoload/vundle.vim
+++ b/autoload/vundle.vim
@@ -6,7 +6,10 @@

 " Plugin Commands
 com! -nargs=+  -bar   Plugin
-\ call vundle#config#plugin(<args>)
+\ call vundle#config#plugin(<f-args>)
+
+com! -nargs=?  -bar   PluginInit
+\ call vundle#config#plugin_init_all(<args>)

 com! -nargs=? -bang -complete=custom,vundle#scripts#complete PluginInstall
 \ call vundle#installer#new('!' == '<bang>', <q-args>)
diff --git a/autoload/vundle/config.vim b/autoload/vundle/config.vim
index e6707a8..4f3ebe6 100644
--- a/autoload/vundle/config.vim
+++ b/autoload/vundle/config.vim
@@ -9,7 +9,7 @@ endf
 func! vundle#config#plugin(arg, ...)
   " If arguments aren't passed in as dict, pack them
   if a:0 > 0 && type(a:1) != type({})
-    let opts = s:pack_dict(a:000)
+    let opts = [s:pack_dict(a:000)]
   else
     let opts = a:000
   endif
@@ -19,6 +19,11 @@ func! vundle#config#plugin(arg, ...)
   return bundle
 endf

+func! vundle#config#plugin_init_all(...)
+  call s:rtp_rm_a()
+  call s:rtp_add_a()
+endf
+
 func! vundle#config#init()
   if !exists('g:bundles') | let g:bundles = [] | endif
   call s:rtp_rm_a()
@@ -46,20 +51,16 @@ func! vundle#config#init_bundle(name, opts)
   return b
 endf

-func! s:pack_dict(...)
+func! s:pack_dict(args)
+  " split key=value arguments into list of [k, v] pairs
+  let kvs = map(copy(a:args),'split(v:val, "=", 1)')
+
   let dict = {}
-  let n_list = []
-
-  for ele in a:000
-    let words = split(ele, '=')
-    if len(words) > 1
-      let dict[words[0]] = words[1]
-    else
-      let n_list = insert(n_list, ele)
-    endif
+  for [k, v] in kvs
+    let dict[k] = v
   endfor

-  return insert(n_list, dict)
+  return dict
 endf

 func! s:parse_options(opts)

And add PluginInit after all Plugin declarations.

Notes:

  • config#plugin accepts <f-args> in order to have arguments (name=blah) quoted
  • s:pack_dict parses arguments into [k, v] pairs
  • constructs option dictionary

@gmarik
Copy link
Contributor

gmarik commented Mar 23, 2014

Also if we're to improve startup speed with introduction of PluginInit (or any other cmd that makes sense) then we have to document it and add to the readme ASAP. Even if it's doing nothing.

That way we can have everyone configured Plugin* migration correctly and be ready for future updates and speed improvements…

@gmarik
Copy link
Contributor

gmarik commented Mar 23, 2014

Found issues(with patch above):

  • rtp='ok vim/' value with spaces errors: E282: Cannot read from ".vim/vimrc"

@gmarik
Copy link
Contributor

gmarik commented Mar 23, 2014

adding k=value seem to be a huge change, maybe we should postpone it…
And think about PluginInit thing first…

@starcraftman
Copy link
Contributor Author

@gmarik

Also if we're to improve startup speed with introduction of PluginInit (or any other cmd that makes sense) then we have to document it and add to the readme ASAP. Even if it's doing nothing.

That way we can have everyone configured Plugin* migration correctly and be ready for future updates and speed improvements…

You have a point here. I'll open up a separate PR with a doc update for the API listing reason to put it. PluginInit name works for me.

Regarding my quick implementation, thanks for pointing out the issues. Didn't know about f-args, still learning I'll have a look at these arg variants. I was mainly just putting it as a sketch to discuss.

@gmarik
Copy link
Contributor

gmarik commented Mar 23, 2014

@starcraftman f-args described just below q-args in :h q-args :).

@jdevera what was your approach implementing key=value syntax?

@jdevera
Copy link
Contributor

jdevera commented Mar 24, 2014

Spec'ing it was as far as I got with this one: jdevera/vundle#5. The idea was to have key=value pairs and interpret a sole key as key=1 and nokey as key=0, just to make it like Vim's settings.

However I did not think of values with spaces at the time 😞

@chiphogg
Copy link
Contributor

We use key=value syntax at Google and have been for some time.

Have you guys considered talking to the google/maktaba people? They've built up a lot of expertise on doing things this way.

@Soares, @dbarnett, and @xanderman would be the ones to talk to.

@dbarnett
Copy link

@chiphogg Related: We're thinking of adding hooks to Glaive so you can configure it to auto-activate plugins using a function of your choosing (through Vundle, VAM, etc.): google/glaive#14. That way you could have just one concise line per plugin to activate and configure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants