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

Migrate from gometalinter.v2 to golangci-lint #249

Merged
merged 5 commits into from
Mar 1, 2019
Merged

Conversation

thrasher-
Copy link
Collaborator

@thrasher- thrasher- commented Feb 28, 2019

Description

Due to gometalinter now being EOL, this PR migrates us over to golangci-linter. The benefits are that it's quicker, supports more linters and is easily customisable by a config file. To start, 19 linters have been enabled but over time we will aim to add more.

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Travis and make check

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules

@codecov-io
Copy link

codecov-io commented Feb 28, 2019

Codecov Report

Merging #249 into master will increase coverage by 0.09%.
The diff coverage is 45.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #249      +/-   ##
==========================================
+ Coverage   42.27%   42.36%   +0.09%     
==========================================
  Files         120      120              
  Lines       26094    26078      -16     
==========================================
+ Hits        11032    11049      +17     
+ Misses      14144    14124      -20     
+ Partials      918      905      -13
Impacted Files Coverage Δ
exchanges/localbitcoins/localbitcoins.go 35.5% <ø> (ø) ⬆️
exchanges/zb/zb_websocket.go 0% <ø> (ø) ⬆️
config/config_encryption.go 74.5% <ø> (ø) ⬆️
currency/coinmarketcap/coinmarketcap.go 57.73% <ø> (ø) ⬆️
restful_server.go 2.5% <0%> (-0.04%) ⬇️
exchanges/coinbasepro/coinbasepro_websocket.go 0% <0%> (ø) ⬆️
exchanges/poloniex/poloniex_wrapper.go 21.05% <0%> (ø) ⬆️
restful_router.go 0% <0%> (ø) ⬆️
exchanges/bitfinex/bitfinex_websocket.go 0% <0%> (ø) ⬆️
events/events.go 0% <0%> (ø) ⬆️
... and 88 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81852f2...7cbe1c8. Read the comment docs.

Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

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

tACK just had a comment.

exchanges/ticker/ticker.go Outdated Show resolved Hide resolved
@thrasher- thrasher- force-pushed the golangci-linter branch 2 times, most recently from 9237b24 to 265b295 Compare February 28, 2019 11:51
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

0 issues found before hitting an error.
Error: Sorry, an error occurred while processing your request. Please try again.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

0 issues found before hitting an error.
Error: Sorry, an error occurred while processing your request. Please try again.

for _, exch := range c.Exchanges {
if exch.Name == exchangeName {
for _, account := range exch.BankAccounts {
for x := range c.Exchanges {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is iterating on the index vs putting it in a variable here better practice, more efficient or both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

An index or pointer is the most efficient, doing it the original way allocates an exchange config object per iteration.

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

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

utACK or whatever
Cool stuff

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

0 issues found before hitting an error.
Error: Sorry, an error occurred while processing your request. Please try again.

Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

1 issues found. Ignoring 0 issues.

@@ -317,10 +318,12 @@ func TestProcessTicker(t *testing.T) { //non-appending function to tickers

for _, test := range testArray {
wg.Add(1)
fatalErr := false
Copy link

Choose a reason for hiding this comment

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

Every exported function in a program should have a doc comment. The first sentence should be a summary that starts with the name (TestProcessTicker) being declared.
From effective go.

currency/forexprovider/fixer.io/fixer.go Outdated Show resolved Hide resolved
exchanges/coinut/coinut.go Outdated Show resolved Hide resolved
@shazbert
Copy link
Collaborator

shazbert commented Mar 1, 2019

tACK

@thrasher- thrasher- merged commit 7dcb1ab into master Mar 1, 2019
@thrasher- thrasher- deleted the golangci-linter branch March 1, 2019 05:10
Copy link

@codelingo codelingo bot left a comment

Choose a reason for hiding this comment

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

0 issues found before hitting an error.
Error: Sorry, an error occurred while processing your request. Please try again.

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.

4 participants