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

Only pass revision argument to diff command when required #70

Merged
merged 1 commit into from Dec 9, 2016

Conversation

vhda
Copy link
Contributor

@vhda vhda commented Dec 7, 2016

Move common diff generation code to local script function.

@vhda
Copy link
Contributor Author

vhda commented Dec 7, 2016

@blueyed There you go. This code is a bit cleaner than what I did this morning.

Copy link
Owner

@tomtom tomtom left a comment

Choose a reason for hiding this comment

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

In general, I prefer this approach. Would you be willing to make the suggested changes? Thanks!

@@ -573,6 +555,33 @@ function! s:GetParam(name, type, default) abort "{{{3
endf


function! s:Diff(filename, vcs_type) "{{{3
let cmdt = s:Config(a:vcs_type).cmd
let rev_arg = s:GetParam('quickfixsigns#vcsdiff#rev_arg', a:vcs_type, s:Config(a:vcs_type).rev_arg)
Copy link
Owner

Choose a reason for hiding this comment

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

rev_arg isn't a (user-configurable) "parameter" but a fixed value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that I should do this?

let rev_arg = s:Config(a:vcs_type).rev_arg

Copy link
Owner

Choose a reason for hiding this comment

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

Yes.


" Create command string
let cmds = printf("%s %s", g:quickfixsigns#vcsdiff#cd, shellescape(dir))
if exists('g:quickfixsigns#vcsdiff#revision') && !empty('g:quickfixsigns#vcsdiff#revision')
Copy link
Owner

Choose a reason for hiding this comment

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

g:quickfixsigns#vcsdiff#revision should be queried as user-configurable parameter.

I'd rather set revision to the formatted revision argument if a revision is specified or an empty string if no rev is specified. This makes the code duplication in the else branch obsolete.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

@@ -437,7 +437,7 @@ g:quickfixsigns#vcsdiff#vcs (default: {...})
cmd ... command templates that generate a unified diff file.
"%s" is replaced with the filename.
dir ... the directory name
revision ... The default revision/branch
rev_arg ... argument to selection revision to diff against
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for updating the help file. The help file is auto-generated from time to time so it's not absolutely necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you use for that purpose? I could benefit from such a flow.

Copy link
Owner

@tomtom tomtom Dec 8, 2016

Choose a reason for hiding this comment

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

I use this ruby script: https://github.com/tomtom/vimtlib/tree/master/ruby

It was a quick hack, and it isn't well maintained, though.

Move common diff generation code to local script function.
@vhda
Copy link
Contributor Author

vhda commented Dec 8, 2016

Updated.

@tomtom tomtom merged commit 116ecc0 into tomtom:master Dec 9, 2016
@tomtom
Copy link
Owner

tomtom commented Dec 9, 2016 via email

@vhda
Copy link
Contributor Author

vhda commented Dec 9, 2016

Thank you for your helpful suggestions!

@blueyed
Copy link
Contributor

blueyed commented Dec 9, 2016

Thanks to both of you from me! :)

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.

None yet

3 participants