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

Add support for Vim-Signify #93

Merged
merged 1 commit into from
Oct 28, 2016
Merged

Conversation

hkupty
Copy link
Contributor

@hkupty hkupty commented Sep 29, 2016

As well as gitgutter, add support for vim-signify.

@jreybert
Copy link
Owner

Thanks for the PR!

But jenkins is positive:

Error detected while processing /home/travis/build/jreybert/vimagit/plugin/magit.vim:
line 74:
E580: :endif without :if: endif

Please fix your commit, preferably with a commit --amend (if error is in last commit) or a rebase --autosquash

@hkupty
Copy link
Contributor Author

hkupty commented Sep 30, 2016

Sorry about that.. It was a bad leftover from previous commit.. Now it should work.

endif
if ( g:magit_refresh_gutter == 1 || g:magit_refresh_gitgutter == 1)

if ( exists("*gitgutter#process_buffer") )
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You save a if statement at runtime, but the evaluation is done at plugin initialization. It means that if vimagit is loaded before gitgutter and/or vim-signify, exists("*gitgutter#process_buffer") will be evaluated to false.

I do not like either the if in the autocmd, but this is the only way to not be dependent on plugin loading order.

Please, only add vim signify support for this PR.

If you find a better way to check gitgutter/vim-signify presence, I'll be happy to merge :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.. That makes sense.

I'll cut this down to solving signify for now.

Aside from the if controversy, are you ok with changing the variable name?

@jreybert
Copy link
Owner

jreybert commented Oct 4, 2016

Excellent!

Could you please update the doc/vimagit.txt and README.md with the new g:magit_refresh_gutter and add a note for the deprecated one.

Thanks again for the PR!

@jreybert
Copy link
Owner

jreybert commented Oct 4, 2016

Argh, I missed the jenkins complaint!

Vim(if):E15: Invalid expression: (exists("_gitgutter#process_buffer")) call gitgutter#process_buffer(bufnr(g:magit_last_updated_buffer), 0) elseif (exists("_sy#util#refresh_windows")) call sy#util#refresh_windows() endif

I'll do the doc update, just fix this for the merge.

@hkupty
Copy link
Contributor Author

hkupty commented Oct 4, 2016

Sorry for taking long. This should be fixed now.

@hkupty
Copy link
Contributor Author

hkupty commented Oct 10, 2016

Can we merge this?

@jreybert
Copy link
Owner

Hi Henry,

I'm very sorry for the delay. I really don't have much time at the moment. It's a pity because you did two nice PRs. You're also the first one to propose PRs touching in the heart of vimagit. That's why I am so careful.

The content of this PR is OK to me. I don't want to discourage you (I really should write some PR guideline). Could you:

  • make the pull request on branch next (mandatory)
  • reduce the number of commits (nice to have)

For the first point, I want to test your feature one week or two before merging it to master. It is mandatory.

For the second point, it is a nice to have for this commit. It will be mandatory for your next PRs. In a PR, I would prefer to not have:

  • add feature foo
  • fix feature foo
    Please use git commit --fixup and git rebase --autosquash.

Thanks for your effort!

@hkupty
Copy link
Contributor Author

hkupty commented Oct 13, 2016

No problem!

I'll address those points later this week, as I'm out on holidays.

@hkupty hkupty changed the base branch from master to next October 27, 2016 18:13
@hkupty
Copy link
Contributor Author

hkupty commented Oct 28, 2016

@jreybert Done

@jreybert jreybert merged commit 55fbadc into jreybert:next Oct 28, 2016
@jreybert
Copy link
Owner

Thanks!

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.

2 participants