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

use Plugin* instead Bundle* #403

Merged
merged 1 commit into from
Mar 23, 2014
Merged

use Plugin* instead Bundle* #403

merged 1 commit into from
Mar 23, 2014

Conversation

gmarik
Copy link
Contributor

@gmarik gmarik commented Mar 18, 2014

  • closes many issues )

@gmarik
Copy link
Contributor Author

gmarik commented Mar 18, 2014

@jdevera @starcraftman please take a look.
Closes

Plugin is now declared with -bar so " is allowed

It's incomplete implementation but the idea is here

TODO:

[] update docs
[] update readme

@gmarik
Copy link
Contributor Author

gmarik commented Mar 18, 2014

So basically the proposal is to rename Bundle* to Plugin* and have aliases like VundleInstall and VundleUpdate

Having Plugin* we can keep Bundle* around for a while without breaking things…

@gmarik gmarik changed the title use Plugin instead Bundle use Plugin* instead Bundle* Mar 18, 2014
@starcraftman
Copy link
Contributor

@gmarik We'll this is a bit unexpected and a large change, but I think it is a welcome one. Will certainly clear up some ambiguity in terminology of docs we can just now refer to everything as plugins instead of bundles/scripts. And people get to make comments on the end.

I assume this will be a rather lengthy deprecation window of months eh?

With regards to Doc update, I guess we can move all the official names to Plugin. Do you want the old help tags to be preserved (i.e. :BundleInstall next to :PluginInstall?) Or should we make a separate doc section explaining the rename and point all Bundle* tags there?

PS: I'm glad regex exists to make that rewrite easy.

@Shougo
Copy link
Contributor

Shougo commented Mar 18, 2014

Having Plugin* we can keep Bundle* around for a while without breaking things…

I think the backward compatibility is important. The old names should not be removed.

@starcraftman
Copy link
Contributor

@gmarik Did a quick initial pass on a fork of plugin of the doc update.

Just need to know what you want done with Plugin/Bundle helptags as I previously asked. Also, do we continue to use bundle folder for plugins? That name seems a bit less apt since we aren't calling commands Bundle*. Though I'm not sure of a good replacement.

My fork: https://github.com/starcraftman/vundle/tree/plugin

@gmarik
Copy link
Contributor Author

gmarik commented Mar 19, 2014

@starcraftman I think bundle/ is fine: why create more issues? :) Otherwise we'd have to rename Vundle to Vplugin for consistency too ^___^

@gmarik
Copy link
Contributor Author

gmarik commented Mar 19, 2014

As for helptags: can we have both for now?
This requires a bit more manual work, but that's the price of transition…

@lucc
Copy link
Contributor

lucc commented Mar 19, 2014

I think the idea for a separate doc section for the old commands is best. You can also add a tag like vundle-interface-change and link to that from the README or from some issues that may arise.

Also we could have a multi-stage deprecation phase like this:

let s:done = 0
function Stage1()
  if s:done
    return
  else
    let s:done = 1
  endif
  echomsg "The Bundle* versions of the commands are deprecated ... bla bla"
endfu
function Stage2()
  if s:done
    return
  else
    let s:done = 1
  endif
  echoerr "The Bundle* versions of the commands are deprecated ... bla bla"
endfu
function Stage3()
  echoerr "The Bundle* versions of the commands are deprecated ... bla bla"
endfu

and with these functions we can define the old Bundle* commands via

com! -nargs=+         Bundle
\ call Stage1() <BAR> call vundle#config#bundle(<args>)

and so on.
So in the early stages the user will get one warning for the first Bundle* command used, later on there will be warnings for every use of a Bundle* command.

@jdevera
Copy link
Contributor

jdevera commented Mar 19, 2014

This is a very welcome change that unblock us greatly. Personally, however, I don't like Plugin for the same reason I never liked Bundle: They don't relate to the name of this plugin.

Why are we not just calling them Vundle ?

@jdevera
Copy link
Contributor

jdevera commented Mar 19, 2014

@lucc I like this multi stage deprecation plan you mention, however we must keep in mind that there is no guarantee that a user will go through all the stages. So somebody whose .vimrc works fine now can decide to update their bundles a couple of years later and we are already in stage3. I'd be okay with that, though.

@jdevera
Copy link
Contributor

jdevera commented Mar 19, 2014

@gmarik Also, if, in the commit message, you specify the issues it fixes, github will close them automatically when the commit is merged. There is a special syntax for it.

I would also suggest that the explanation of why this change is happening be included in the commit message itself :)

@starcraftman
Copy link
Contributor

@jdevera I definitely agree we should put an explanation in the extended commit message section.

As to the naming of Plugin* vs Vundle* a good issue to raise. To me Plugin seems a bit more natural as it better describes what is happpening. PluginInstall clearly conveys I'm installing plugins. VundleInstall on the other hand doesn't as clearly convey the same meaning. I do get the idea of using the logical namespace though and it may look a bit odd to not namespacing with Vundle.

@starcraftman
Copy link
Contributor

@gmarik Doc helptags updated, added transition section at end of vundle.txt. Also updated README with a note on interface change at end of about. If you are happy with the changes, feel free to pull from my fork.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 19, 2014

@jdevera my reasoning for Plugin is simple: when anyone reads
Plugin 'tpope/vim-pathogen'
it's obvious right away unlike
Vundle 'tpope/vim-pathogen
which requires knowing what Vundle is. Using Vundle for everything is confusing atleast. Vundle is a manager. Plugin describes what it is - a plugin.
Vundle is the last one on my replacement list for Bundle

Aware about having closes/fixes in commit messages, but since it's just a WIP commit i didn't bother to include it.
Once we have all ready we can squash everything and have a proper commit.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 19, 2014

@lucc I was thinking about just adding deprecation message to a window whenever VundleInstall/List/Search is opened. Syntax highlighted. Simpler, harder to miss.

And yeah, someone may be just bypassing stages…

@jdevera
Copy link
Contributor

jdevera commented Mar 20, 2014

Ok I see. It's readable to have those 'Plugin' lines. And it is nice because then some other plugin manager can be used if it has the same syntax.

However, I don't think that has to match the command names. I still think we should have Vundle in the name of the commands. VundleInstall, VundleUpdate. This plugin, Vundle, is a tool, not an common text editing operation, as such I, as a user, am less confused if what I have to run includes the tool name.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 20, 2014

I'm ok with VundleInstall/Update/Search too!

@starcraftman
Copy link
Contributor

@gmarik Agreed, aliasing some of the commands to Vundle makes sense.

So to summarise, we currently have the new aliases:
Plugin, Plugins, PluginList, PluginInstall(!), PluginUpdate, PluginClean(!), PluginSearch(!), PluginDocs

And we can add the following aliases for Vundle*:
VundleInstall(!), VundleUpdate, VundleSearch(!), VundleClean(!), VundleDocs

I omitted Plugin, Plugins and PluginList because those don't make much sense if we substitute Vundle for Plugin.

@jdevera
Copy link
Contributor

jdevera commented Mar 20, 2014

I wouldn't add any of the Plugin* commands, except Plugin. I don't know what to do about Bundles, as a command entered interactively by the user, I think it should start with Vundle*, but VundleList is already taken. Perhaps VundleKnown?

@starcraftman
Copy link
Contributor

@jdevera Is there any harm in providing user choice on these two valid APIs? No reason we can't keep them side by side in vundle.vim. In terms of docs, can simply put both help tags in the vimdoc and default to showing Plugin.

I see the Plugin* namespace as being a more naturalistic API. If I tell someone who configured plugins in their vimrc that the command PluginInstall installs them, that makes sense. Similarly, Plugins listing all available plugins via search makes sense, as does PluginList.

As a programmer though, I strongly understand the desire to namespace to Vundle for api consistency. Thus we can keep most of the commands aliased to Vundle* as I wrote above.

Does that make sense to you?

@jdevera
Copy link
Contributor

jdevera commented Mar 20, 2014

I guess I agree, after some more thought. I initially thought it was somewhat arrogant to call the command PluginInstall as if Vundle were the one and only plugin manager. Then I realised that whichever plugin manager a user installs is the one and only manager for them, as people are not going to install two of them at a time 😉

So I'm cool with having cohabiting commands.

@starcraftman
Copy link
Contributor

@jdevera Happy we got that sorted then. I'll update my documentation branch a little later to reflect the change. Also need to rebase to latest stuff.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 20, 2014

👍

@starcraftman
Copy link
Contributor

@gmarik Updated my fork with better explanation on interface change and rebased. Did a quick test, there may be a slight issue. The PluginUpdate command seems to work too fast, think something's off.

Link: https://github.com/starcraftman/vundle/tree/plugin

@starcraftman
Copy link
Contributor

@gmarik @jdevera Oh and I updated my fork with the Vundle aliases for all commands except Plugin, Plugins and PluginList. Made a handy table in the vimdoc to summarize the new api. https://github.com/starcraftman/vundle/blob/plugin/doc/vundle.txt#L356-L364

Commands other than PluginUpdate seem to work, not sure why it broke. Will look at it later if someone else doesn't by then.

@jdevera
Copy link
Contributor

jdevera commented Mar 21, 2014

Nit picking here, please forgive me, but I think passive voice is better for docs :)

@jdevera
Copy link
Contributor

jdevera commented Mar 21, 2014

By the way @starcraftman since you have commit access you can make the changes directly on the 'plugin' branch of this repo, the all gets added to the pull request. Then in the end, when merging to master, we do it with the command line and squash all commits into one. That would make it easier to test, since I can easily checkout PR's on my live Vundle, or even the 'plugin' branch.

@starcraftman
Copy link
Contributor

@jdevera

Nit picking here, please forgive me, but I think passive voice is better for docs :)

Interesting. At University my engineering writing teachers always insisted active was better for documentation, especially instructional ones where an author is directing or explaining steps. I just got used to writing like that lately.

By the way @starcraftman since you have commit access you can make the changes directly on the 'plugin' branch of this repo, the all gets added to the pull request. Then in the end, when merging to master, we do it with the command line and squash all commits into one. That would make it easier to test, since I can easily checkout PR's on my live Vundle, or even the 'plugin' branch.

Aye, I know that flow. I guess I'm just a little timid to just push onto other people's branches. I'll push my fork over the main plugin branch now so we can look at the update thing.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 22, 2014

@starcraftman

I'd like to know how much difference that gives on startup

That's what pathogen does, you can profile its startup time to get an idea )
Just for 'Plugin'

I think key=value syntax is much more readable and easier to type!

@starcraftman
Copy link
Contributor

@gmarik @jdevera Time profiling of Vundle vs Pathogen on my high end i7. This is first time startup after boot so has to start YCM and load a lot. Command used is time vim +qall. Seems from limited testing it might be worth doing the work. My vimrc if you are curious about what got loaded. https://github.com/starcraftman/.my_scripts/blob/master/vim_and_bash_config/.vimrc

Vundle:
real 0m4.172s
user 0m0.064s
sys 0m0.036s

Subsequent calls are around 130-140ms.

Pathogen.
real 0m1.313s
user 0m0.084s
sys 0m0.020s

Subsequent pathogen start ups take around 0.100s real or less.

@ches
Copy link

ches commented Mar 22, 2014

Just briefly chiming in FWIW on interactive installer: given the lack of version locking, personally I do use it frequently, for updating -- unqualified BundleUpdate is fairly insane if you use a fair number of plugins, somebody's upstream is bound to break your environment before long. But I often update a handful of plugins at once, usually launching a fresh Vim instance to do some spot-checking along the way. For that workflow, BundleList and invoking I on an educated selection of things I know to be stable is very convenient.

Unsubscribing because I think there will likely be a fair bit of continued discussion here that I likely won't be able to keep up with, but feel free to @ mention me if there's anything more you'd appreciate a user's feedback on. Thanks for your work on Vundle folks!

@gmarik
Copy link
Contributor Author

gmarik commented Mar 22, 2014

@ches cool, thanks for feedback!

@starcraftman
Copy link
Contributor

@gmarik @jdevera Added possible implementation for key/value pairs. Any args passed to a Plugin line of form 'key=value' get repacked into a dict as expected by Bundle traditionally. Args that don't contain an = get ignored.

@jdevera
Copy link
Contributor

jdevera commented Mar 23, 2014

I think we should discuss the new argument format in a separate PR, and keep this one for command renaming only. Thoughts?

@starcraftman
Copy link
Contributor

@jdevera @gmarik Ya, I guess we have gotten a bit sidetracked, we should probably consider these two suggestions in a separate API Improvement PR. I'll pop the changesets off this branch. Is there anything else then to do for this Plugin modification or do we want to merge it? Key/value support does seem nifty though.

@jdevera
Copy link
Contributor

jdevera commented Mar 23, 2014

I'd say we're good to go with this one. If you want to merge please use squash generously :)

- closes many issues )
@starcraftman
Copy link
Contributor

@jdevera Aye, I'm agreed. Rebase has been performed, and now going to do a manual merge to fix the conflicts.

@starcraftman starcraftman merged commit 0521de9 into master Mar 23, 2014
@starcraftman
Copy link
Contributor

Damn, forgot to mark the fixes issues in the comment will go manually do that now.

@gmarik
Copy link
Contributor Author

gmarik commented Mar 23, 2014

Good job guys! 👍

@gmarik gmarik deleted the plugin branch March 23, 2014 16:55
@jdevera jdevera mentioned this pull request Apr 4, 2014
pityonline added a commit to pityonline/config that referenced this pull request Apr 15, 2018
pityonline added a commit to pityonline/config that referenced this pull request Apr 15, 2018
pityonline added a commit to pityonline/config that referenced this pull request Apr 15, 2018
pityonline added a commit to pityonline/config that referenced this pull request May 19, 2018
pityonline added a commit to pityonline/config that referenced this pull request May 19, 2018
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

Successfully merging this pull request may close these issues.

6 participants