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

don't error out early, if git not exists #392

Closed
wants to merge 1 commit into from
Closed

don't error out early, if git not exists #392

wants to merge 1 commit into from

Conversation

chrisbra
Copy link

Even if git is not available, one can still use plug.vim
for managing the runtime path.
This allows, to share the same config with several vim instances, some
of which, might not have vim installed (or not have it in $PATH). The
commands :Plug and :PlugClean seem to be safe even in this case.

Unfortunately in that case, plug.vim errors out on every start.

So let's just move the check for the existance of git to the place,
where it is actually needed.

In theory, we can also just move the check :if excutable('git') around
the definition of the all commands that need git, so they would be only
defined on systems that have git installed. But I actually like the way,
how the commands will always be defined, but they error out once one
tries to execute them.

Even if git is not available, one can still use plug.vim
for managing the runtime path.
This allows, to share the same config with several vim instances, some
of which, might not have vim installed (or not have it in $PATH). The
commands `:Plug` and `:PlugClean` seem to be safe even in this case.

Unfortunately in that case, plug.vim errors out on every start.

So let's just move the check for the existance of git to the place,
where it is actually needed.

In theory, we can also just move the check `:if excutable('git')` around
the definition of the all commands that need git, so they would be only
defined on systems that have git installed. But I actually like the way,
how the commands will always be defined, but they error out once one
tries to execute them.
@vheon
Copy link
Contributor

vheon commented Jan 27, 2016

Even if git is not available, one can still use plug.vim
for managing the runtime path.

Can't you do it using vim-plug as it is right now? All you get is an error (IMHO the message is not correct but that is another matter)

Unfortunately in that case, plug.vim errors out on every start.

If a user don't want to see the error can he just use silent! call plug#begin()?

But I actually like the way,
how the commands will always be defined, but they error out once one
tries to execute them.

I believe that is a personal preference. I prefer not to have the command if I can't run it (like vim-fugitive for example)

@junegunn
Copy link
Owner

If you git blame the lines and see the commit message, you'll notice that the behavior is by design.

c1bbbaf

I prefer the current approach in that

  • it informs the user up-front of possible misconfiguration
  • and the user can choose to turn off the warning with silent!

Having crippled commands available or not is just a matter of preference as @vheon pointed out and thus I don't see a good reason to suddenly change our decision at this point. I can understand that you prefer the other way though. And PlugClean also requires git for checking the remote URI.

I'm closing this but, I think we have to mention silent! workaround in the error message. Thanks.

@junegunn junegunn closed this Jan 28, 2016
junegunn added a commit that referenced this pull request Jan 28, 2016
@chrisbra
Copy link
Author

On Mi, 27 Jan 2016, Junegunn Choi wrote:

I prefer the current approach in that

  • it informs the user up-front of possible misconfiguration
  • and the user can choose to turn off the warning with silent!

I don't like this solution for various resons:

  • :sil! hides other other error messages. I consider that bad
    style and would only consider this as a temporary work-around.
  • This means with a shared vimrc, I would have either to duplicate the
    current logic in my .vimrc:
let git=executable('git')
exe printf("%s%s", git?'':'sil!', "call plug#begin('~/.vim/bundle')")

or to always unconditionally use :sil!. I really don't like either
solution.

  • Third, my approach would have silently worked, no need for a
    configuration. That is better from a users point of view.

Having crippled commands available or not is just a matter of
preference as @vheon pointed out and thus I don't see a good reason to
suddenly change our decision at this point. I can understand that you
prefer the other way though.

I see it differently. The plugin defines the commands as its interfaces,
therefore they should be always available. Thus you don't later wonder,
why you only have the command :Plug but why are you missing all the
other :Plug commands. You won't find out, until you read the source.

With the approach I suggested, you will always know exactly, why the
command does not work. I consider this better from a user side. BTW:
This is how vim does it itself with commands that have been disabled at
compile time.

And PlugClean also requires git for
checking the remote URI.

Oh, I didn't notice. It seemed to work fine from my test of a vim
without an installed git.

I'm closing this but, I think we have to mention silent! workaround
in the error message. Thanks.

You are closing my issues too fast for my taste and i am slowly getting
the feeling that feedback isn't appreciated (and I am only looking into
vim-plug for about a day now).

@junegunn
Copy link
Owner

:sil! hides other other error messages. I consider that bad style and would only consider this as a temporary work-around.
...
to always unconditionally use :sil!. I really don't like either solution.

You have a point, but plug#begin() is pretty simple and not much can go wrong really and so this boils down to a matter of preference though you would probably disagree.

You won't find out, until you read the source.

Now you can, thanks to the updated message.

You are closing my issues too fast for my taste and i am slowly getting the feeling that feedback isn't appreciated (and I am only looking into vim-plug for about a day now).

I appreciate your suggestions and feedbacks, but you see, vim-plug has been around for more than two years by now and most of the points you make are extensively discussed and concluded several times in that past, and we know the ups and downs of different approaches. Please don't assume that we mindlessly throw things into this project without proper assessment. I don't see a good reason to go all over again and explain to you why we made those choices just because it doesn't fit your taste. And your patch didn't even work.

@chrisbra
Copy link
Author

On Mi, 27 Jan 2016, Junegunn Choi wrote:

:sil! hides other other error messages. I consider that bad style and would only consider this as a temporary work-around.
...
to always unconditionally use :sil!. I really don't like either solution.

You have a point, but plug#begin() is pretty simple and not much can go wrong really and so this boils down to a matter of preference though you would probably disagree.

You won't find out, until you read the source.

Now you can, thanks to the updated message.

Until you forgot about :sil! and much later wonder why :PlugInstall does
not work...

You are closing my issues too fast for my taste and i am slowly getting the feeling that feedback isn't appreciated (and I am only looking into vim-plug for about a day now).

I appreciate your suggestions and feedbacks, but you see, vim-plug has been around for more than two years by now and most of the points you make are extensively discussed and concluded several times in that past, and we know the ups and downs of different approaches. Please don't assume that we mindlessly throw things into this project without proper assessment. I don't see a good reason to go all over again and explain to you why we made those choices just because it doesn't fit your taste. And your patch didn't even work.

I am not sure, what argument are you making here. I have been around for
much longer than 10 years in the vim community some of my plugins are
almost as old. That doesn't mean that everything is correct in there as
well and I still consider feedback for those plugins as valuable.

Also I am not sure why you are saying my patch did not work. It did,
because I tested it before submitting the PR. But hey, your project, you
make the rules. Will go with something different then.

@junegunn
Copy link
Owner

I'm saying what you're proposing are not new to us, thus fast feedback was possible which most people appreciate.

In your patch, s:git_exists() returns 0 if git is available. All the commands return without doing anything.

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.

3 participants