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

Check we have `goimports` #453

Merged
merged 4 commits into from Oct 18, 2018

Conversation

Projects
None yet
5 participants
@tcharding
Copy link
Contributor

commented Oct 17, 2018

bin/check_goimports succeeds even when goimports is not installed. This is because a shell variable gets set to the shell error for 'command not found'. Subsequent check on that variable is logically incorrect because of this. We can fix this by asserting we have the command goimports before calling it.

This is a reasonably common thing to do so lets add a function to bin/helpers/ to do the check. Also test the new function.

Patch 1: Adds the helper function need_cmd.
Patch 2: Adds extendable test script bin/helpers/test.sh and tests need_cmd
Patch 3: Adds statement to check_goimports: need_cmd goimports

Fixes: #450

Please note: This is untested since I don't have access to the CI infrastructure. Applying this patch will cause check_goimports to fail if goimports is not installed on the CI infrastructure (by design).

Signed-off-by: Tobin C. Harding me@tobin.cc

@tcharding tcharding requested review from tadovas and Waldz as code owners Oct 17, 2018

exit 0

This comment has been minimized.

Copy link
@tcharding

tcharding Oct 17, 2018

Author Contributor

I couldn't seem to get an editor to edit this file without adding these 'exit 0' lines to the diff. I tried emacs and vim?

This comment has been minimized.

Copy link
@soffokl

soffokl Oct 17, 2018

Member

It's ok, it's even better to have a new line at the end of the file.

This comment has been minimized.

Copy link
@tcharding

tcharding Oct 17, 2018

Author Contributor

+1 that is my opinion also. I'll add the newline in a separate patch and force push.

This comment has been minimized.

Copy link
@tcharding

tcharding Oct 18, 2018

Author Contributor

Added newline.

@@ -13,6 +13,9 @@
#> bin/check_goimports ./communication/...

source bin/helpers/output.sh

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member
Suggested change
source bin/helpers/output.sh
set -e
source bin/helpers/output.sh

This comment has been minimized.

Copy link
@Waldz

Waldz Oct 17, 2018

Member

missing set -e in header, so that script would fail with missing goimports or any other error

This comment has been minimized.

Copy link
@tcharding

tcharding Oct 17, 2018

Author Contributor

set -e seems to be shunned by some people [1] but I see it is use widely here. Whats the ruling on this? If you guys want set -e I'll close this and do a patch adding set -e to check_goimports.

[1] http://mywiki.wooledge.org/BashFAQ/105

This comment has been minimized.

Copy link
@tcharding

tcharding Oct 18, 2018

Author Contributor

Used this solution as advised. PR has been updated.

thanks

@Waldz Waldz requested review from zolia, vkuznecovas and soffokl Oct 17, 2018

@zolia
Copy link
Member

left a comment

lgtm

tcharding added some commits Oct 18, 2018

Add newline to end of shell script
Currently bin/check_goimports does not end with a newline.  In keeping
with UNIX folklore we should have a newline on the end of a shell script
file.

Add newline to end of shell script file.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
Add missing 'set -e' to shell script
Currently bin/check_goimports is missing `set -e` line.

Add `set -e`

Fixes: Issue #450

Signed-off-by: Tobin C. Harding <me@tobin.cc>

Waldz added some commits Oct 18, 2018

@Waldz

Waldz approved these changes Oct 18, 2018

@Waldz Waldz merged commit a4927a1 into mysteriumnetwork:master Oct 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tcharding tcharding deleted the tcharding:functions branch Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.