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.

Sign up for GitHub

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

Jump to bottom

Refactor global variables to autoload variables #450

Merged
merged 5 commits into from
Mar 21, 2015
Merged

Refactor global variables to autoload variables #450

merged 5 commits into from
Mar 21, 2015

Conversation

lucc
Copy link
Contributor

@lucc lucc commented Apr 24, 2014

This is just an idea. If you like it I will continue to work on it. See commit message for more.

@jdevera
Copy link
Contributor

jdevera commented Apr 24, 2014

I'm rather curious about the rationale behind this change. What does it give us?

@jdevera
Copy link
Contributor

jdevera commented Apr 24, 2014

Ok I'm cool with that, but we must also be able to consume all the current variables that are already in place.

Perpahs we could have a function to read them all from whatever sources and put them in a dictionary that we can use everywhere, so we don't have to do 20 ifs all over the place.

Let's hear what @gmarik and @starcraftman have to say.

@jdevera
Copy link
Contributor

jdevera commented Apr 24, 2014

Also, since you are "in the loop", it's more convenient to open your PR's against next.

@lucc
Copy link
Contributor Author

lucc commented Apr 24, 2014

somehow my comment was duplicated and now that I removed the duplicate I cant see it anymore. Will repost.

EDIT: Can I change the PR to be for next?

@lucc
Copy link
Contributor Author

lucc commented Apr 24, 2014

The idea is to not pollute the global namespace with dfferent variables. A common prefix will make it clear where the variable belongs.

@lucc
Copy link
Contributor Author

lucc commented Apr 24, 2014

I am not sure about renameing g:vundle_default_git_proto to g:vundle#default_git_proto as this is practiacally a change of the public api. We could go the same way as with :Bundle and keep the old variable functional and add a note to the docs.

But if we do change it we might also give it a better name (at the moment it is long but uses an abreviation). some suggestions:

  • g:vundle#git_proto
  • g:vundle#git_protocol
  • g:vundle#default_git_protocol

Also I would vote to rename g:vundle#bundle_dir to g:vundle#prefix. This is closer to the prefix one finds in ./configure scripts and makefiles.

@lucc lucc changed the title [WIP] Redactor global variables to autoload variables [WIP] Refactor global variables to autoload variables Apr 25, 2014
@lucc
Copy link
Contributor Author

lucc commented Apr 25, 2014

or even g:vundle#protocol

@lucc lucc changed the title [WIP] Refactor global variables to autoload variables Refactor global variables to autoload variables Apr 25, 2014
@starcraftman
Copy link
Contributor

@lucc Need to look this over further before more detailed comment but I believe we've been fairly consistent in not breaking any already existing apis. So yes, we'd have to make the new name an alias.

@lucc
Copy link
Contributor Author

lucc commented Apr 27, 2014

To me a15b29d looks like it can be merged. It will change all global variables except for g:vundle_default_git_proto. I would live with this inconsistency and make a separated PR for this variable as it is the only one affecting the public api. Then we can merge this PR more easily and discuss deprecation notices and docs in the other PR.

Also renaming of global variables like vundle#bundle_dir to vundle#prefix can be discussed separately.

@lucc
Copy link
Contributor Author

lucc commented Apr 29, 2014

I rebased this on next and squashed the commits. It should only be merged after v0.10.2.

@jdevera jdevera mentioned this pull request May 1, 2014
Open
@jdevera
Copy link
Contributor

jdevera commented May 1, 2014

@lucc I have re-read everything and I take back my objection. I'll be merging this as is.

@lucc
Copy link
Contributor Author

lucc commented May 1, 2014

nice to hear, I was half hearty thinking this over but did not come up with a good idea.

@jdevera
Copy link
Contributor

jdevera commented May 1, 2014

My idea for moving vundle#updated_bundles, vundle#log and s:changelog out of vundle#rc() was that these variables are independent of the arguments of the function. So setting them in here was counter intuitive for me.

This made more sense this time than the first time I read it, apologies.

@lucc lucc mentioned this pull request May 2, 2014
Closed
@lucc lucc mentioned this pull request Feb 20, 2015
Open
lucc added 2 commits March 3, 2015 10:40
@lucc
Remove unneeded variable setting.
cad5f50 
The variable is script local to autoload/vundle/scripts.vim since #468.
@lucc
Refactor global variables into script local variables.
088295d 
These variables only occur in one file each.  By making them script local
variables this is "documented" in the code.  At the same time the global
namespace is polluted less.

Changed:
    g:bundle_names          -> s:bundle_names
    g:vundle_last_status    -> s:last_status
    g:vundle_log_file       -> s:log_file
    g:vundle_view           -> s:view
@lucc
Copy link
Contributor Author

lucc commented Mar 3, 2015

I have rebased (actually rewritten) these commits on top of the current master. I have also split the work into different commits to better show what is done.

@gmarik
Copy link
Contributor

gmarik commented Mar 3, 2015

@lucc looks much nicer!

@jdevera jdevera self-assigned this Mar 4, 2015
@lucc
Allow multiple calls to VundleLog.
5f27abb 
If it was loaded, unload the log buffer before editing it.  Otherwise the
editing command can "hang" if the user has `set hidden`.

This problem was originally discovered with the VundleChangelog command and
the analogous fix was applied in 7d9b10.  See github issue #468 for more.
@lucc
Copy link
Contributor Author

lucc commented Mar 4, 2015

@jdevera I tried to apply your comment (sadly it is lost now, as the commit is gone)

lucc added 2 commits March 4, 2015 13:12
@lucc
Refactor global variables into autoload variables.
356f245 
All global variables that are not part of the public API (mentioned in the
documentation) are turned into autoload variables.  This is intended to give
all global variables defined by Vundle.vim a common prefix.  The variable
g:default_git_proto is part of the public API and is therefor not changed.
This is the only exception.

Changed:
   g:bundle_dir            -> vundle#bundle_dir
   g:bundles               -> vundle#bundles
   g:updated_bundles       -> vundle#updated_bundles
   g:vundle_lazy_load      -> vundle#lazy_load
   g:vundle_log            -> vundle#log
Unchanged:
   g:default_git_proto
@lucc
Set default values for autoload variables.
16237bb 
The setting of the default values for the autoload variables is moved out of
any function.  One reason being that these settings do not depend on the
argument of the function.  The second being that Vim will source the autoload
script if an undefined autoload variable is referenced and the file is
expected to define the variable (see :help autoload).
@jdevera
Copy link
Contributor

jdevera commented Mar 21, 2015

I wish I remembered that comment :)

@jdevera
Copy link
Contributor

jdevera commented Mar 21, 2015

I gave this a bit of a run and it seems solid to me. Merging.

jdevera added a commit that referenced this pull request Mar 21, 2015
@jdevera
Merge pull request #450 from lucc/autoload-vars
34a3077 
Refactor global variables to autoload variables
@jdevera jdevera merged commit 34a3077 into VundleVim:master Mar 21, 2015
@lucc lucc deleted the autoload-vars branch March 22, 2015 11:45
jasoncodes added a commit to jasoncodes/dotfiles that referenced this pull request Mar 30, 2015
@jasoncodes
Update for new Vundle public API
02f81be 
VundleVim/Vundle.vim#450 killed a bunch of globals.
jasoncodes added a commit to jasoncodes/hackling-dotfiles that referenced this pull request Mar 30, 2015
@jasoncodes
Update for new Vundle public API
a511cfb 
VundleVim/Vundle.vim#450 killed a bunch of globals.
@jasoncodes jasoncodes mentioned this pull request Mar 30, 2015
Merged
@lucc lucc unassigned jdevera Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lucc @jdevera @starcraftman @gmarik