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 build tags to many commands #1705

Merged
merged 5 commits into from
Apr 5, 2018
Merged

Add build tags to many commands #1705

merged 5 commits into from
Apr 5, 2018

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Mar 5, 2018

Fixes #1464

@fatih
Copy link
Owner

fatih commented Mar 6, 2018

We should keep :GoBuildTags. :GoBuildTags is dynamic, and sometimes it's handy to have it. go_build_tags on the other hand is permanent, for people who always work on files that have it stick in their files.

@arp242
Copy link
Contributor Author

arp242 commented Mar 6, 2018

:GoBuildTags is dynamic, and sometimes it's handy to have it. go_build_tags on the other hand is permanent, for people who always work on files that have it stick in their files.

What do you mean with "dynamic" and "permanent" in this context? All :GoBuildTags does is set go_build_tags, so it behaves the same?

@fatih
Copy link
Owner

fatih commented Mar 6, 2018

Yeah I know that it sets it, but it's get resets when you restart vim. Hence the word dynamic. Of course it's not the best solution, because you override the prior value which was set via go_build_tags. The better solution would be to not override go_build_tags , but not sure if that is worth the complexity. But the command is really handy, sometimes you see a new file that has a build tag and you want to change it quickly. I think we should have it, but maybe we should change how it behaves. Dunno.

@bhcleek
Copy link
Collaborator

bhcleek commented Mar 6, 2018

FWIW, I agree with @fatih; I use :GoBuildTags frequently. As a general guideline, anytime we expect users to be able to change a config value during a session, I'm in favor of providing a convenience function to set it.

@arp242 arp242 force-pushed the build-tags branch 2 times, most recently from f0db7a5 to e5a0c8f Compare April 4, 2018 22:31
@arp242 arp242 removed the wip label Apr 4, 2018
@arp242
Copy link
Contributor Author

arp242 commented Apr 4, 2018

Okay, fair enough. I added :GoBuildTags back. clarified the documentation, and rebased on mater after the config changes. Should be good to merge now.

Also fixes :GoBuildTags "" to clear the tags. Looks like this didn't work anymore (probably regression from Billy's config PR? Didn't investigate too deeply).

if exists('g:go_build_tags')
unlet g:go_build_tags
endif
if a:value is '' || a:value is '""' || a:value is "''"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The conversion from '' and "" to the empty string should happen in go#cmd#BuildTags. I'll submit a separate PR to do that so it's resolved separately...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I merged your PR and rebased on master.

@bhcleek
Copy link
Collaborator

bhcleek commented Apr 4, 2018

assuming builds pass, lgtm

Fixes #1464

Also fixes `:GoBuildTags ""` to clear the tags. Looks like this didn't
work anymore (probably regression from Billy's config PR? Didn't
investigate too deeply).
@arp242 arp242 merged commit ae41131 into master Apr 5, 2018
@arp242 arp242 deleted the build-tags branch April 5, 2018 12:13
@jdef
Copy link

jdef commented Apr 6, 2018

[EDIT] My project uses tags sparingly, and I'm running golang 1.10. When I updated vim-go to master the :GoCoverage command complained that tags may only be specified once - the current file/package do not specify any tags. When I rolled back vim-go to the commit just prior to his one, :GoCoverage worked again.

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.

4 participants