Fix linters' warnings #388

b00ris · 2017-10-09T13:20:50Z

This PR fixes gometalinter warnings ( #281) except dubl linter.

divan · 2017-10-09T15:57:22Z

I think this PR should not touch tests, until #375 is merged. Probably would be hard to resolve the conflicts after the huge changes and file movements in mentioned PR. LGTM otherwise..

tiabc · 2017-10-09T16:26:37Z

cmd/statusd/utils.go

@@ -835,7 +837,7 @@ func testCompleteMultipleQueuedTransactions(t *testing.T) bool {

 	//  this call blocks, and should return when DiscardQueuedTransaction() for a given tx id is called
 	sendTx := func() {
-		txHashCheck, err := statusAPI.SendTransaction(nil, common.SendTxArgs{
+		txHashCheck, err := statusAPI.SendTransaction(context.TODO(), common.SendTxArgs{


Good catch! By the way, why TODO instead of Background?

@tiabc Both of them are empty context.
context.Background - need empty context.
context.TODO - it's unclear which Context to use.
It helps us to find all unclear context usage later.

But here we have a test. Do we want context.TODO to draw our attention here? I guess context.Background is perfectly fine or let's use context.WithTimeout to prevent from running forever.

Scope of this issue could grow fast. I use it, because the issue is fix linters warnings. I think Adding context.WithTimeout to tests is separated issue and we could do it, if we need.

tiabc · 2017-10-09T16:54:15Z

Setting #371 as a blocker to merge it after it.

b00ris · 2017-10-09T16:55:02Z

@divan most of the affected files are related to tests =(

tiabc · 2017-10-09T16:58:16Z

geth/account/accounts.go

@@ -218,7 +218,8 @@ func (m *Manager) SelectAccount(address, password string) error {
 		return err
 	}

-	if err := whisperService.SelectKeyPair(accountKey.PrivateKey); err != nil {
+	err = whisperService.SelectKeyPair(accountKey.PrivateKey)
+	if err != nil {


Please, refrain from refactoring in regular issues in the future. Even if you don't like the code. We only do refactoring in refactoring issues to ease merging.

There is no refactoring. I've fixed variable shadowing

tiabc · 2017-10-09T16:58:44Z

geth/api/api.go

@@ -195,6 +195,7 @@ func (api *StatusAPI) JailBaseJS(js string) {
 	api.b.jailManager.BaseJS(js)
 }

+// Notify (push notification and mailbox??)


Notify sends a push notification to the device with the given token.

tiabc · 2017-10-09T16:59:35Z

geth/api/api_test.go

@@ -123,5 +123,5 @@ func (s *APITestSuite) TestRaceConditions() {
 	}

 	time.Sleep(2 * time.Second) // so that we see some logs
-	s.api.StopNode()            // just in case we have a node running
+	_ = s.api.StopNode()        // just in case we have a node running


Well, this is a test and not of s.api.StopNode and we don't really care.

it's only fixing errcheck linter warning.

//nolint: errcheck?

It's another way, but i've changed it to nolint. It's more faster way to find all nolint tags.

tiabc · 2017-10-09T17:05:42Z

geth/jail/internal/fetch/fetch.go

 		return err
 	}

-	return nil
+	_, err = t.cb.Call(otto.NullValue(), arguments...)
+	return err
 }

 func (t *fetchTask) Cancel() {


Hasn't golint blamed this line?

tiabc

First of all, outstanding execution!

Please, resolve written comments and re-file the PR from the local repo. Also, the branch name should be bugfix/fix-linters-warnings-387.

I'll revisit the PR after I merge #375 and you resolve appeared conflicts, if any.

tiabc · 2017-10-09T17:11:44Z

geth/jail/internal/timers/timers.go

+	}
+
+	err = vm.Set("clearImmediate", clearTimeout)
+	return err


Just return vm.Set... as you've already done in other places?

In that case it's more clear for me. A lot of errchecks.

tiabc · 2017-10-09T17:15:36Z

geth/jail/internal/promise/promise_test.go

@@ -88,7 +90,7 @@ func (s *PromiseSuite) SetupTest() {
 	s.vm = vm.New(o)
 	s.loop = loop.New(s.vm)

-	go s.loop.Run()
+	go s.loop.Run(context.TODO()) //nolint: errcheck


New argument? How did it work before?

It was not working after merge.

tiabc · 2017-10-09T17:16:19Z

geth/node/manager_test.go

@@ -441,7 +441,7 @@ func (s *ManagerTestSuite) TestNodeStartCrash() {
 	require.NoError(err)

 	// start node outside the manager (on the same port), so that manager node.Start() method fails
-	outsideNode, err := node.MakeNode(nodeConfig)
+	outsideNode, _ := node.MakeNode(nodeConfig)


Nice catch! Fixed.

tiabc · 2017-10-09T17:20:36Z

geth/txqueue/txqueue_manager_test.go

@@ -137,9 +137,9 @@ func (s *TxQueueTestSuite) TestCompleteTransactionMultipleTimes() {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
-			_, err := txQueueManager.CompleteTransaction(tx.ID, TestConfig.Account1.Password)
+			_, errCompleteTransaction := txQueueManager.CompleteTransaction(tx.ID, TestConfig.Account1.Password)


Why so long?

Why not? It's obvious name.

Yes but why so long? Why not just e? Usually, the longer a variable name, the broader its scope.

It could be e. But.. Why not?) In that case what should be next name of error after e? ee?
errCompleteTransaction is obvious name. It's simple. You don't need to comment it.

There won't be another case. If there's, the name will be just longer. Anyway, it's just a matter or taste, I prefer longer variables for broader scope and shorter for narrower. Following and knowing this rule just makes you understand existing code faster.

Is this rule fixed in our docs? Should we check "a matter or taste" on code review?

Sorry?
Docs is not what's doled out to us, this is what we ourselves create during open discussions. If there's something you follow which makes code better, why shouldn't you share it?
I'm not asking to fix that based on my matter of taste, by that I'm saying my comment isn't relevant any more as the matter isn't worth the efforts discussing it.

i'll try to describe better my point.
As far as i know we are going to make oun community. If so we need strict CR rules on which all major contributors agreed. If we all agred that "less scope = shorter name", this should be fixed in doc to be project standard for core developers and all new contributers.

I agree but your question was strange given that we don't have such a document yet at all and I never asked to fix anything due to just my taste.
And not all comments are major, some of them are just for discussion like this one and written along with major comments. Or they're just some questions.

tiabc · 2017-10-09T17:22:22Z

static/config/linter_exclude_list.txt

+comment on exported function StopCPUProfiling should be of the form "StopCPUProfiling ..." (golint)
+comment on exported function WriteHeapProfile should be of the form "WriteHeapProfile ..." (golint)
+comment on exported function Notify should be of the form "Notify ..." (golint)
+type name will be used as node.NodeManager by other packages, and that stutters; consider calling this Manager (golint)


I think it's more reasonable to refactor this. Can you delete this line and add a note to #200?

I fully support this. I'll add note to #200.

adambabik · 2017-10-09T22:18:45Z

Makefile

@@ -95,47 +95,47 @@ lint-cur:

 lint: ##@tests Run meta linter on code


I wonder if we can replace all these lines with:

gometalinter $(LINT_EXCLUDE) --disable-all --enable=vet --enabled=gofmt --enable=goimports [--enable=...] cmd/... geth/...

And I guess this linter_exclude_list.txt can be removed as we can use // nolint: golint for individual functions?

Yes, we could call gometalinter as you offered, but for me it's less convinient to use.
About linter_exclude_list.txt i fully agree with you. I had to switch to #383 and push as is.

I think i could fix it after #383 and #375

tiabc · 2017-10-11T14:24:22Z

Please, resolve conflicts and let's merge it.

b00ris · 2017-10-12T10:32:42Z

@tiabc I'm going to do it after deploying faucet for rinkeby at #383

b00ris · 2017-10-19T21:25:31Z

Made separated #417 PR from local repository.
My miss. I've done it from my fork.

fixed make lint warnings cleared linter_exclude_list.txt removed some commented code fixed comments from #388

fix linters except dupl

c5aa7fd

divan changed the title ~~fix linters except dupl~~ Fix linters' warnings Oct 9, 2017

tiabc reviewed Oct 9, 2017

View reviewed changes

adambabik reviewed Oct 9, 2017

View reviewed changes

b00ris mentioned this pull request Oct 19, 2017

Fix make lint warnings #417

Merged

b00ris closed this Oct 19, 2017

tiabc pushed a commit that referenced this pull request Oct 20, 2017

Fix make lint warnings (#417)

e911666

fixed make lint warnings cleared linter_exclude_list.txt removed some commented code fixed comments from #388

b00ris mentioned this pull request Oct 26, 2017

Fix linter warning across the whole project #435

Closed

		@@ -95,47 +95,47 @@ lint-cur:

		lint: ##@tests Run meta linter on code

Fix linters' warnings #388

Fix linters' warnings #388

Conversation

b00ris commented Oct 9, 2017 • edited Loading

divan commented Oct 9, 2017

Choose a reason for hiding this comment

b00ris Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiabc commented Oct 9, 2017

b00ris commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiabc Oct 9, 2017 • edited Loading

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

Choose a reason for hiding this comment

tiabc 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiabc commented Oct 11, 2017

b00ris commented Oct 12, 2017

b00ris commented Oct 19, 2017

b00ris commented Oct 9, 2017 •

edited

Loading

b00ris Oct 9, 2017 •

edited

Loading

tiabc Oct 9, 2017 •

edited

Loading