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

golangci-lint/CI: Bump versions and introduce new linters #798

Merged
merged 15 commits into from
Oct 14, 2021

Conversation

thrasher-
Copy link
Collaborator

PR Description

Bumps golangci-lint/CI Go versions and introduces a few new linters!

Type of change

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

How has this been tested

  • go test ./... -race
  • golangci-lint run

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
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Github Actions/AppVeyor with my changes
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Oct 9, 2021

Codecov Report

Merging #798 (f536cc4) into master (5208d21) will decrease coverage by 0.30%.
The diff coverage is 27.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #798      +/-   ##
==========================================
- Coverage   43.68%   43.38%   -0.31%     
==========================================
  Files         311      311              
  Lines       80042    80287     +245     
==========================================
- Hits        34967    34833     -134     
- Misses      39936    40214     +278     
- Partials     5139     5240     +101     
Impacted Files Coverage Δ
common/file/file.go 65.51% <0.00%> (-0.59%) ⬇️
common/timeperiods/timeperiods.go 100.00% <ø> (ø)
...vider/currencyconverterapi/currencyconverterapi.go 0.00% <0.00%> (ø)
currency/storage.go 29.44% <ø> (+0.07%) ⬆️
engine/engine.go 28.16% <0.00%> (-0.13%) ⬇️
engine/rpcserver.go 30.95% <0.00%> (-0.30%) ⬇️
exchanges/binance/binance_wrapper.go 41.18% <0.00%> (+0.13%) ⬆️
exchanges/bithumb/bithumb.go 56.96% <0.00%> (-0.53%) ⬇️
exchanges/bithumb/bithumb_ws_orderbook.go 23.70% <0.00%> (ø)
exchanges/bitmex/bitmex_wrapper.go 45.43% <ø> (-0.58%) ⬇️
... and 98 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 5208d21...f536cc4. 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.

This is an awesome update! 🚀 x 100. Just a couple of nits below.

cmd/exchange_wrapper_issues/main.go Outdated Show resolved Hide resolved
cmd/exchange_wrapper_issues/main.go Outdated Show resolved Hide resolved
common/cache/lru.go Show resolved Hide resolved
common/cache/lru.go Show resolved Hide resolved
currency/pairs.go Outdated Show resolved Hide resolved
exchanges/lbank/lbank.go Show resolved Hide resolved
exchanges/orderbook/node.go Outdated Show resolved Hide resolved
exchanges/request/request.go Show resolved Hide resolved
exchanges/stream/stream_match_test.go Outdated Show resolved Hide resolved
exchanges/zb/zb.go Outdated Show resolved Hide resolved
@shazbert shazbert added the reconstructing Based on PR feedback, this is currently being reworked and is not to be merged label Oct 10, 2021
@thrasher- thrasher- added review me This pull request is ready for review and removed reconstructing Based on PR feedback, this is currently being reworked and is not to be merged labels Oct 11, 2021
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.

tested approved. This is a great update! 🚀

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.

Lovely big PR that looks like it wasn't fun to do! Thanks!
My concerns are only len related and only because its type checking after type checking. Not much of a big deal

One extra thing as a favour is that there are a few instances of the following: *int64(time.Millisecond) where the implementation can now be changed from time.Unix(0, ...*int64(time.Millisecond)) to time.UnixMilli(...)
For example, bitfinex_websocket time.Unix(0, int64(element[0].(float64))*int64(time.Millisecond)),

edit: as this is about linters, the linters themselves pass. So I'm happy for you to say "later" and change this to tACK

exchanges/bitfinex/bitfinex_websocket.go Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
exchanges/bitfinex/bitfinex_websocket.go Outdated Show resolved Hide resolved
gctscript/modules/ta/indicators/bbands.go Show resolved Hide resolved
gctscript/modules/ta/indicators/atr.go Show resolved Hide resolved
gctscript/modules/ta/indicators/macd.go Show resolved Hide resolved
gctscript/modules/ta/indicators/mfi.go Show resolved Hide resolved
gctscript/modules/ta/indicators/obv.go Show resolved Hide resolved
gctscript/modules/ta/indicators/rsi.go Show resolved Hide resolved
gctscript/modules/ta/indicators/sma.go Show resolved Hide resolved
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.

tACK! Thanks for making those changes + the unixMilli ones too!

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.

tACK changes!

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.

ACK changes

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! 🥇

@thrasher- thrasher- merged commit f0d45aa into thrasher-corp:master Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants