-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Conversation
gmarik
commented
Mar 18, 2014
- closes many issues )
@jdevera @starcraftman please take a look.
It's incomplete implementation but the idea is here TODO: [] update docs |
So basically the proposal is to rename Having |
@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. |
I think the backward compatibility is important. The old names should not be removed. |
@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. |
@starcraftman I think |
As for helptags: can we have both for now? |
I think the idea for a separate doc section for the old commands is best. You can also add a tag like 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. |
This is a very welcome change that unblock us greatly. Personally, however, I don't like Why are we not just calling them |
@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. |
@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 :) |
@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. |
@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. |
@jdevera my reasoning for Plugin is simple: when anyone reads Aware about having closes/fixes in commit messages, but since it's just a WIP commit i didn't bother to include it. |
@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… |
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. |
I'm ok with VundleInstall/Update/Search too! |
@gmarik Agreed, aliasing some of the commands to Vundle makes sense. So to summarise, we currently have the new aliases: And we can add the following aliases for Vundle*: I omitted Plugin, Plugins and PluginList because those don't make much sense if we substitute Vundle for Plugin. |
I wouldn't add any of the Plugin* commands, except |
@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? |
I guess I agree, after some more thought. I initially thought it was somewhat arrogant to call the command So I'm cool with having cohabiting commands. |
@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 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. |
@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. |
Nit picking here, please forgive me, but I think passive voice is better for docs :) |
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. |
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.
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. |
That's what pathogen does, you can profile its startup time to get an idea ) I think key=value syntax is much more readable and easier to type! |
@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 Vundle: Subsequent calls are around 130-140ms. Pathogen. Subsequent pathogen start ups take around 0.100s real or less. |
Just briefly chiming in FWIW on interactive installer: given the lack of version locking, personally I do use it frequently, for updating -- unqualified 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! |
@ches cool, thanks for feedback! |
I think we should discuss the new argument format in a separate PR, and keep this one for command renaming only. Thoughts? |
@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. |
I'd say we're good to go with this one. If you want to merge please use squash generously :) |
- closes many issues )
@jdevera Aye, I'm agreed. Rebase has been performed, and now going to do a manual merge to fix the conflicts. |
Damn, forgot to mark the fixes issues in the comment will go manually do that now. |
Good job guys! 👍 |