Skip to content

Fix lint warnings #160

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

Merged
merged 1 commit into from
Apr 28, 2022
Merged

Fix lint warnings #160

merged 1 commit into from
Apr 28, 2022

Conversation

vr009
Copy link

@vr009 vr009 commented Apr 8, 2022

code health: fix all places highlighted by linter

Changed the warning's suppression in check.yaml. The suppression of the rule
errcheck may be removed after adding errors check in all methods
with calling encodeXxx inside. See details below.

Suppressed the highlighting lacks of error's check in all methods,
having encodeXxx inside. For now those methods are not able to
return any error due to internal implementation of writer
interface (see smallbuf.go). For future, if the implementation of the writer
will be changed, and there will be a need to check errors, we must think
about how to say to compiler that the error check is 'unlikely' for keeping performance
(If there will be any affect on it).

Fixed the use of time package API in all places with calls of
time.Now().Sub(). Now time package propose the explicit time.Until() and time.Since().

Replaced all calls of Errorf() with Fatalf() in tests, where it is
needed. That change prevents nil dereferences below in the code and
stops the test execution, where it is expected in tests.

Suppressed the highlighting of all unused constants and functions with //nolint comment.

Fixed the calling of Fatalf() from non-testing goroutine in queue tests.
It is not a valid way to stop test from another goroutine.

Fixed the gofmt-highlighted places in test_helpers/pool_helper.go and
connection_pool/const.go.

Added instructions to CONTRIBUTING.md how to run CI-linter locally.

Closes #142
Closes #150

@vr009 vr009 requested a review from ligurio April 8, 2022 09:29
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch 5 times, most recently from 40838c4 to a5e6d31 Compare April 13, 2022 09:08
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from a5e6d31 to 3b1689b Compare April 13, 2022 18:50
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch 2 times, most recently from 6715ffc to 8437785 Compare April 20, 2022 08:34
@vr009 vr009 marked this pull request as ready for review April 20, 2022 08:50
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch 2 times, most recently from 269b0b8 to 6bd1dd1 Compare April 20, 2022 13:22
@vr009 vr009 requested a review from ligurio April 21, 2022 06:52
Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

Replaced some calls of Errorf() with Fatalf() in tests. That change
prevents nil dereferences below in the code.

It looks strange that goling-ci warns for a few t.Errorf because we have about several dozens t.Errorf() in tests (and we have plans to fix it in #150):

sergeyb@pony:~/sources/go/src/github.com/tarantool/go-tarantool$ find . -name "*_test.go" | xargs grep "t.Errorf" | wc -l
316

@ligurio
Copy link
Member

ligurio commented Apr 21, 2022

in commit message:

Relates to #142

It should be "Closes #142"

Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

In general, I'm ok with changes, but please fix commit message and godoc comments before merge.

Copy link
Member

@ligurio ligurio left a comment

Choose a reason for hiding this comment

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

In general, I'm ok with changes, but please fix commit message and godoc comments before merge.

@DifferentialOrange
Copy link
Member

code health: fix all places highlighted by linter

It is good that there are many people working on Go connector right now, but it also means their work should be synchronized. For example, I think that big nice PR #148 will be merged before that one, and there could be some new lint warnings. So it seems that you need to sync with other contributors so

  • this PR fixes all warnings in current master
  • all other PRs are rebased to new master and contain no new warnings

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

I have left a several comments. As I remember from talking with Sergey, there really isn't many questionable places left, so I think the review won't spend too many time.

@DifferentialOrange
Copy link
Member

Please, rebase to master. It is possible that master discovery commit contains some new warnings

@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch 4 times, most recently from 7f2defc to 5820237 Compare April 26, 2022 11:22
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from 5820237 to 044ecba Compare April 26, 2022 13:55
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

I'm ok after resolving my comments.

@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from 044ecba to 0c3fabb Compare April 26, 2022 16:36
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from 0c3fabb to 7e5c0da Compare April 26, 2022 17:00
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from 7e5c0da to 99bad6d Compare April 28, 2022 07:25
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch 5 times, most recently from 633e0cc to 224e999 Compare April 28, 2022 18:52
Changed the warning's suppression in check.yaml. The suppression
of the rule errcheck may be removed after adding errors check
in all methods calling encodeXxx inside. See details below.

Suppressed the highlighting lacks of error's check in all methods,
having encodeXxx inside. For now those methods are not able to
return any error due to internal implementation of writer
interface (see smallbuf.go). For future, if the implementation of the writer
will be changed, and there will be a need to check errors, we must think
about how to say to compiler that the error check is 'unlikely' for keeping performance
(If there will be any affect on it).

Fixed the use of time package API in all places with calls of
time.Now().Sub(). Now time package propose the explicit time.Until().

Replaced all calls of Errorf() with Fatalf() in tests, where it is
needed. That change prevents nil dereferences below in the code and
stops the test execution, where it is expected in tests.

Suppressed the highlighting of all unused constants and functions with //nolint comment.

Fixed the calling of Fatalf() from non-testing goroutine in queue tests.
It is not a valid way to stop test from another goroutine.

Fixed the gofmt-highlighted places in test_helpers/pool_helper.go and
connection_pool/const.go.

Added instructions to CONTRIBUTING.md how to run CI-linter locally.

Closes #142
Closes #150
@vr009 vr009 force-pushed the vr009/gh-142-fix-lint-warnings branch from 224e999 to 2fa5d44 Compare April 28, 2022 18:55
@vr009 vr009 merged commit de95e31 into master Apr 28, 2022
@vr009 vr009 deleted the vr009/gh-142-fix-lint-warnings branch April 28, 2022 19:03
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.

Errorf not finishes test execution Create a lint CI job
4 participants