improved compatibility: vundle should now support vim >= 7.0 #354
Conversation
* added new functions to detach caller from having to call
fnameescape() (which isn't available until vim-7.2):
* s:compat_rtp_rm_entry(dirs): abstracts 'set rtp-=LIST';
* s:compat_rtp_addset_entry(dirs, addset_operator):
abstracts 'set rtp+=LIST', 'set rtp^=LIST',
'set rtp=LIST';
* created autoload/vundle/compat.vim: "compatibility" module
(implements functions not available in previous vim versions);
(code taken from my previous commit in 'autoload/vundle/config.vim',
then modified and extended);
* added compatibility function for shellescape();
* autoload/vundle/config.vim: tidied up so that it's a bit closer to
the file upstream;
* (previous commit) but added a few performance improvements (avoid
unnecessary calls and/or operations);
* altered other scripts to use 'vundle#compat#*()' functions instead of
those not necessarily available in previous vim versions;
|
Hey, thanks for this PR! I have a couple of questions/comments, I'll leave some of those inline. Mainly my question here is under which environments you have tested this change. Have you given it a spin under windows? |
| let l:runtimepath_orig = &rtp | ||
| let l:processed_flag = 0 | ||
| let l:elem_separator = ',' | ||
| for l:process_stage in range(1, 4) |
There was a problem hiding this comment.
Curiosity: In the previous PR this used to be a simple series of if/elseif.../else. Why did it change to a loop with stages? I think the previous code was clearer.
There was a problem hiding this comment.
Fair enough. My reasons for this loop are:
- the previous code(IIRC) only attempted to do one of several approaches (the new one allows you to attempt one, then decide it didn't do what you wanted, and keep trying for other methods);
- the new code allows for easy re-ordering, just by changing the constante the 'l:process_stage' variable is compared against;
- the new code allows for more code to be easily added in the 'if l:processed_flag' section -- not that I needed anything so far, but it allows for common code, too (for example, you could flag something from more than one stage and do it at the bottom of the loop -- then decide the value of l:processed_flag, for example). It's all about easing future fixes (which I assumed will happen at some point :-) );
ghost
assigned 
|
No problem. I think 'vundle' is really useful, and I wanted to contribute to make it a bit better. I've only tested on Linux (versions 7.0.0, 7.1.0, 7.2.0, 7.3.something, 7.4.0), as testing on Windows is a little bit awkward for me at the moment (I don't run Windows at home). I can give it a spin on a Windows machine at some point or another. In any case, I've stated my "dev test cases" for you or anyone else to reproduce. I thought that before taking the pull request, people would branch their working copy to a new branch, pull from my repo, then test the changes, then make some modifications (if needed), then take the pull request, then "merge" the new "master" into the working copy (which would be an "already up-to-date" merge), then push. It does not look that easy when you describe it all here, though ;-) |
|
I wish we had such a systematic approach. We are lucky to have a few collaborators, unfortunately none of them runs windows. So since I have assigned this PR to myself I'll run it for a few days when it's all done and try to make it sweat a bit. I'll try to get older vim instances set up this week to test this. And will also try to get someone with windows to give it a go. |
* use local variable in filter expression (filter()); * moved comments regarding future changes to the bottom of the file;
|
I'll send another pull request, including changes triggered by your review comments. |
I've seen that vundle seemed to only run on vim >= 7.2 (mainly due to the reliance on fnameescape()), so I've decided to create the 'autoload/vundle/compat.vim' file to provide implementations for both "new" (and "supporting") vim versions, and also for those vim versions that didn't provide those functions.
I've tested using BundleList, BundleInstall, BundleSearch, manually getting the list of packages ('R' on the BundleList). It all seems to work fine (some of the plugins that you can load through 'Bundle' aren't really that good at detecting whether vim supports them or not).
These changes don't seem to have "worsened" vundle in any perceivable way (not at least in the way I've been using it), so I reckon you should be happy to pull :-)