Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. #2885

helinwang · 2017-07-14T22:36:24Z

No description provided.

wangkuiyi

Cool change!

wangkuiyi · 2017-07-15T01:05:36Z

paddle/scripts/travis/check_style.sh

@@ -13,6 +13,12 @@ export PATH=/usr/bin:$PATH
 pre-commit install
 clang-format --version

+# set up go environment for running gometalinter


Who would call check_style.sh?

Oh. I see. I recommended to call check_style.sh from Travis CI.

wangkuiyi

LGTM!

gongweibao · 2017-07-15T01:49:57Z

.pre-commit-config.yaml

    hooks:
      -   id: go-fmt
-          files: (.*\.go)
+          types: [go]
      -   id: go-lint


用了gometalinter应该就不用再单独使用golint了。
https://github.com/alecthomas/gometalinter#quickstart

@gongweibao done.

gongweibao · 2017-07-15T01:51:40Z

paddle/scripts/travis/check_style.sh

+
+echo "gopath: $GOPATH"
+echo "pwd: `pwd`"
+find ./go


这个是做什么？是测试代码？

maybe @helinwang wants to listing the directory to log file. That's reasonable.

@gongweibao @dzhwinter 不好意思，是测试的，不知为何在我本地能运行，在travis不行，加了写代码测试了一下。

@gongweibao @dzhwinter all removed, found the issue:
go tool的对象代码（比如go build xxx）最好是在$GOPATH里面，不然会有各种问题（vendor directory ignored, no caching of built object files...)。我是调travis gometalinter的时候出现的这个问题，如果运行对象不在$GOPATH中，gometalinter一直过不了。改成了这样才过的：https://github.com/PaddlePaddle/pre-commit-golang/blob/master/run-gometalinter.sh#L5

gongweibao · 2017-07-15T01:54:32Z

paddle/scripts/travis/check_style.sh

+echo "gopath: $GOPATH"
+echo "pwd: `pwd`"
+find ./go
+cd go; gometalinter --vendor --disable-all --enable errcheck ./...; cd -


cd go; gometalinter --vendor --disable-all --enable=errcheck --enable=golint ./... ; cd -

我们是否也把vet检查加上？

@gongweibao ~~go vet 有一些正确的地方过不了。~~

Here is the message showing go vet fails:

gometalinter --vendor --disable-all --enable errcheck --enable vet --enable golint ./... master/c/client.go:26::error: possible misuse of unsafe.Pointer (vet) pserver/optimizer.go:17::error: possible misuse of unsafe.Pointer (vet) pserver/client/c/cclient.go:37::error: possible misuse of unsafe.Pointer (vet)

@gongweibao , 修复了，其实go vet都报对了。加上go vet了。

dzhwinter

LGTM!

dzhwinter · 2017-07-15T02:17:49Z

go/pserver/service.go

@@ -219,7 +219,7 @@ func (s *Service) GetParam(name string, parameter *Parameter) error {
 }

 // pserver save checkpoint
-func (s *Service) doCheckpoint() error {
+func (s *Service) doCheckpoint() (err error) {


a smart feature of named return value!
seems that I ignored so much return error when saving the checkpoint.

dzhwinter · 2017-07-15T02:32:06Z

paddle/scripts/travis/check_style.sh

+
+echo "gopath: $GOPATH"
+echo "pwd: `pwd`"
+find ./go


maybe @helinwang wants to listing the directory to log file. That's reasonable.

dzhwinter · 2017-07-15T02:33:17Z

paddle/scripts/travis/check_style.sh

+
+echo "gopath: $GOPATH"
+echo "pwd: `pwd`"
+find ./go


we need to filter out the vendor package file. change to

find ./go -not -path "./go/vendor/*"

@dzhwinter good point! Please see https://github.com/PaddlePaddle/pre-commit-golang/blob/master/run-gometalinter.sh , --vendor already ignore the vendor folder.

Unchecked errors could be handled by: cd go; gometalinter --vendor --disable-all --enable errcheck $(glide nv)

gongweibao

LGTM

helinwang changed the title ~~add error check to pre-commit-hook, fix all go unchecked errors.~~ Add error check to pre-commit-hook, fix all go unchecked errors. Jul 14, 2017

helinwang force-pushed the handle_err branch from de7dff8 to 0ac51a5 Compare July 14, 2017 22:41

helinwang changed the title ~~Add error check to pre-commit-hook, fix all go unchecked errors.~~ Add error check to pre-commit-hook, fix all Go unchecked errors. Jul 14, 2017

helinwang force-pushed the handle_err branch from 0ac51a5 to 11a2326 Compare July 14, 2017 22:43

helinwang changed the title ~~Add error check to pre-commit-hook, fix all Go unchecked errors.~~ Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. Jul 14, 2017

helinwang force-pushed the handle_err branch 6 times, most recently from d7adc54 to 08e7d2a Compare July 15, 2017 00:13

helinwang requested review from dzhwinter and gongweibao July 15, 2017 00:25

wangkuiyi reviewed Jul 15, 2017

View reviewed changes

helinwang mentioned this pull request Jul 15, 2017

fix travis check style for Go #2887

Merged

wangkuiyi approved these changes Jul 15, 2017

View reviewed changes

helinwang force-pushed the handle_err branch 4 times, most recently from 3c78242 to beae398 Compare July 15, 2017 01:40

gongweibao requested changes Jul 15, 2017

View reviewed changes

dzhwinter approved these changes Jul 15, 2017

View reviewed changes

helinwang added 3 commits July 17, 2017 12:04

Handle all unchecked errors

2b1cac4

Unchecked errors could be handled by: cd go; gometalinter --vendor --disable-all --enable errcheck $(glide nv)

add gometalinter/errcheck into pre-commit

065e566

fix golint errors

5d7bccb

helinwang force-pushed the handle_err branch 2 times, most recently from faa803b to c791f90 Compare July 17, 2017 20:41

Fix Go pre-commit

37624b3

helinwang force-pushed the handle_err branch from c791f90 to 37624b3 Compare July 17, 2017 20:44

add more linters, fix errors found by them.

25e5794

gongweibao approved these changes Jul 18, 2017

View reviewed changes

helinwang merged commit 2db1b68 into PaddlePaddle:develop Jul 18, 2017

helinwang deleted the handle_err branch July 18, 2017 06:06

heavengate pushed a commit to heavengate/Paddle that referenced this pull request Aug 16, 2021

add ags module (PaddlePaddle#2885)

c7c0568

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. #2885

Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. #2885

helinwang commented Jul 14, 2017

wangkuiyi left a comment

wangkuiyi Jul 15, 2017

wangkuiyi Jul 15, 2017

wangkuiyi left a comment

gongweibao Jul 15, 2017

helinwang Jul 17, 2017

gongweibao Jul 15, 2017

dzhwinter Jul 15, 2017

helinwang Jul 15, 2017

helinwang Jul 17, 2017

gongweibao Jul 15, 2017 •

edited

Loading

gongweibao Jul 15, 2017

helinwang Jul 15, 2017 •

edited

Loading

helinwang Jul 17, 2017

helinwang Jul 17, 2017 •

edited

Loading

gongweibao Jul 18, 2017

dzhwinter left a comment

dzhwinter Jul 15, 2017

dzhwinter Jul 15, 2017

dzhwinter Jul 15, 2017

helinwang Jul 15, 2017

dzhwinter Jul 16, 2017

gongweibao left a comment

Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. #2885

Add gometalinter errcheck to pre-commit-hook, fix all Go unchecked errors. #2885

Conversation

helinwang commented Jul 14, 2017

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangkuiyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Jul 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

helinwang Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzhwinter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gongweibao left a comment

Choose a reason for hiding this comment

gongweibao Jul 15, 2017 •

edited

Loading

helinwang Jul 15, 2017 •

edited

Loading

helinwang Jul 17, 2017 •

edited

Loading