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

Set GOPATH for both possible go guru execution paths #1193

Merged
merged 3 commits into from
Feb 19, 2017

Conversation

jblebrun
Copy link
Contributor

@jblebrun jblebrun commented Feb 2, 2017

No description provided.

Copy link
Owner

@fatih fatih left a comment

Choose a reason for hiding this comment

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

lgtm, just couple of small changes 👍

@@ -217,11 +213,19 @@ endfunc

" run_guru runs the given guru argument
function! s:run_guru(args) abort

Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this newline please

if go#util#has_job()
return s:async_guru(a:args)
let abort = s:async_guru(a:args)
Copy link
Owner

Choose a reason for hiding this comment

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

This has several meanings. For async it's the job id however for sync_guru it's the output. So the caller defines what to look. Can you rename it to res as for result? I think that would make more sense.

Copy link
Contributor Author

@jblebrun jblebrun Feb 2, 2017

Choose a reason for hiding this comment

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

I have no idea why I called it abort. I think I was distracted by the abort in the function signature.

On closer examination... does the result need to be returned at all? No callers seem to use it. Can we just call the appropriate function and then return nothing here?

Copy link
Owner

Choose a reason for hiding this comment

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

I was returning with the previous one, as we might need it in the future and it's handy. You can also just return however I'm not sure how to unset it easily (as Vim doesn't have defer like we have in Go)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've left it and changed the name to res as you suggested.

let $GOPATH = old_gopath

return abort

Copy link
Owner

Choose a reason for hiding this comment

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

Can you remove this newline as well please?

endif

return s:sync_guru(a:args)
let res = s:sync_guru(a:args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This line should now be in an else block so that guru isn't called twice when async_guru was called.

@fatih fatih merged commit 71efb2e into fatih:master Feb 19, 2017
@fatih
Copy link
Owner

fatih commented Feb 19, 2017

Thanks @jblebrun

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