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

optionally install golang test dependencies #1003

Merged
merged 1 commit into from Jul 12, 2016

Conversation

Projects
None yet
4 participants
@jvshahid
Contributor

jvshahid commented Jul 5, 2016

use the flycheck-go-build-install-deps flag to optionally install test dependencies. this will improve the time it takes to run flycheck

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 6, 2016

@flycheck/go Could you take a look?

flycheck.el Outdated
@@ -6747,7 +6747,7 @@ See URL `http://golang.org/cmd/go'."
;; `temporary-file-name'.
;; TODO: Switch to `null-device'` when < Go 1.6 support is removed.
;; See: https://github.com/flycheck/flycheck/issues/838
:command ("go" "test" "-c" "-o" temporary-file-name)
:command ("go" "test" (option-flag "-i" flycheck-go-build-install-deps) "-c" "-o" temporary-file-name)

This comment has been minimized.

@lunaryorn

lunaryorn Jul 6, 2016

Contributor

Please wrap this line, it's too long. Put the (option-flag …) part on a separate line, and the arguments after it on another separator line.

@jvshahid jvshahid force-pushed the jvshahid:go-test-install-deps branch from 6b0b9da to 4b32954 Jul 6, 2016

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 6, 2016

@lunaryorn done

@dominikh

This comment has been minimized.

Member

dominikh commented Jul 6, 2016

LGTM

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 6, 2016

@jvshahid Please add a note about this change to CHANGES.rst and update the documentation of this option in doc/languages.rst 😊

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 6, 2016

now that i'm looking at the code again, should the name of the option be flycheck-go-build-install-deps or have test in it, i.e. flycheck-go-test-install-deps ?

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 6, 2016

pushed the change to the docs. let me know if i should create a separate option for go-test

@@ -379,7 +379,8 @@ to view the docstring of the syntax checker. Likewise, you may use
.. option:: flycheck-go-build-install-deps
Whether to install dependencies while checking.
Whether to install dependencies while checking. **Note** this option

This comment has been minimized.

@lunaryorn

lunaryorn Jul 7, 2016

Contributor

Better write … while checking with go-buildorgo-test.

@@ -389,6 +390,11 @@ to view the docstring of the syntax checker. Likewise, you may use
Check syntax and types of Go tests with the `Go compiler`_.
.. option:: flycheck-go-build-install-deps

This comment has been minimized.

@lunaryorn

lunaryorn Jul 7, 2016

Contributor

Please add a :noindex: option to this directive. You document the same symbol twice, or it'll mess up cross-referencing.

.. option:: flycheck-go-build-install-deps
Whether to install dependencies while checking. **Note** this option
also affects `go-build`

This comment has been minimized.

@lunaryorn

lunaryorn Jul 7, 2016

Contributor

Don't repeat the documentation here. Instead, with :noindex: as in the comment above, just say See flycheck-go-build-install-deps. and it'll cross-reference the real docs.

flycheck.el Outdated
@@ -6678,7 +6678,7 @@ See URL `https://golang.org/cmd/go/' and URL
:face (if have-vet 'success '(bold error)))))))
(flycheck-def-option-var flycheck-go-build-install-deps nil go-build

This comment has been minimized.

@lunaryorn

lunaryorn Jul 7, 2016

Contributor

Since the option now covers go-test as well you'll need to turn go-build into (go-build go-test) so that the option appears in the docstrings of both syntax checkers.

CHANGES.rst Outdated
@@ -1,6 +1,11 @@
29-cvs (in development)
=======================
- New features:

This comment has been minimized.

@lunaryorn

lunaryorn Jul 7, 2016

Contributor

The changelog's been updated meanwhile. Please rebase on master and merge the changes.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 7, 2016

@jvshahid Some minor issues with the docs are left. Please address these, then rebase onto current master and squash your commits. Thank you :)

@jvshahid jvshahid force-pushed the jvshahid:go-test-install-deps branch from 1bc793e to bd4f057 Jul 7, 2016

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 7, 2016

@lunaryorn thanks for reviewing the pr. all requested changes have been taken care of.

CHANGES.rst Outdated
@@ -5,6 +5,8 @@
- Add ``:default-directory`` option to ``flycheck-define-command-checker``
[GH-973]
- ``flycheck-go-build-install-deps`` turn on dependency installation for ``go test``

This comment has been minimized.

@lunaryorn

lunaryorn Jul 12, 2016

Contributor

turn -> turns :)

@@ -379,7 +379,8 @@ to view the docstring of the syntax checker. Likewise, you may use
.. option:: flycheck-go-build-install-deps
Whether to install dependencies while checking.
Whether to install dependencies while checking while checking with

This comment has been minimized.

@lunaryorn

lunaryorn Jul 12, 2016

Contributor

"while checking" appears twice :)

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 12, 2016

@jvshahid There are a two spelling issues left. Please fix these, and then rebase onto master to resolve the conflict in the changelog.

@jvshahid jvshahid force-pushed the jvshahid:go-test-install-deps branch from bd4f057 to 2eea00f Jul 12, 2016

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 12, 2016

@lunaryorn fixed all the typos mentioned above

CHANGES.rst Outdated
@@ -6,6 +6,9 @@
- Add ``:working-directory`` option to ``flycheck-define-command-checker``
[GH-973] [GH-1012]
- ``flycheck-go-build-install-deps`` turns on dependency installation for ``go test``

This comment has been minimized.

@lunaryorn

lunaryorn Jul 12, 2016

Contributor

Sorry, I forgot one thing: Could you please remove the blank line above this line? In ReST there shouldn't be blank lines within lists, only between different nesting levels.

optionally install golang test dependencies
use the `flycheck-go-build-install-deps' flag to optionally install test
dependencies. this will improve the time it takes to run flycheck

@jvshahid jvshahid force-pushed the jvshahid:go-test-install-deps branch from 2eea00f to 07f3b8d Jul 12, 2016

@jvshahid

This comment has been minimized.

Contributor

jvshahid commented Jul 12, 2016

Done

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 12, 2016

LGTM. @cpitclaudel Would you take a look?

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Jul 12, 2016

LGTM. Thanks for this patch!

@lunaryorn lunaryorn merged commit a1e7669 into flycheck:master Jul 12, 2016

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jul 12, 2016

Merged. Thank you very much for this patch 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment