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

CI/build: Update Go version, linters and fix minor issues #1473

Merged
merged 4 commits into from
Feb 14, 2024

Conversation

thrasher-
Copy link
Collaborator

@thrasher- thrasher- commented Feb 8, 2024

PR Description

Bumps Go to version 1.22
Bumps golangci-lint to v1.56.1
Linter changes:

  • Prefer errors.New when no formatting is needed over fmt.Errorf
  • Prefer string comparison checks over len, there's no compiler performance difference now but makes code more clear. Refer to Russ Cox's explanation who's the co-creator of Go:
If I'm about to look at element x I typically write
len(s) > x, even for x == 0, but if I care about
"is it this specific string" I tend to write s == "".

It's reasonable to assume that a mature compiler will compile
len(s) == 0 and s == "" into the same, efficient code.
Right now 6g etc do compile s == "" into a function call
while len(s) == 0 is not, but that's been on my to-do list to fix.

Make the code clear.
  • Uses strconv and string addition for performance where possible
  • Fixes testify expected vs actual issuess across the codebase impacting quite a few tests
  • Better usage of testify test funcs
  • Some hugeParam and rangeVal fixes

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • 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 with my changes
  • Any dependent changes have been merged and published in downstream modules

@thrasher- thrasher- requested a review from a team February 8, 2024 03:51
@thrasher- thrasher- self-assigned this Feb 8, 2024
@thrasher- thrasher- added the review me This pull request is ready for review label Feb 8, 2024
Copy link

codecov bot commented Feb 8, 2024

Codecov Report

Attention: 198 lines in your changes are missing coverage. Please review.

Comparison is base (6a425e6) 44.01% compared to head (175c584) 37.78%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1473      +/-   ##
==========================================
- Coverage   44.01%   37.78%   -6.23%     
==========================================
  Files         363      411      +48     
  Lines      143949   147817    +3868     
==========================================
- Hits        63353    55856    -7497     
- Misses      72973    84185   +11212     
- Partials     7623     7776     +153     
Files Coverage Δ
backtester/common/common.go 66.20% <100.00%> (+6.53%) ⬆️
common/convert/convert.go 94.89% <100.00%> (-0.27%) ⬇️
communications/telegram/telegram.go 48.57% <100.00%> (-0.21%) ⬇️
exchanges/binanceus/binanceus_types.go 100.00% <ø> (ø)
exchanges/bithumb/bithumb_websocket.go 48.14% <100.00%> (+5.29%) ⬆️
exchanges/bybit/bybit_types.go 73.33% <ø> (+6.66%) ⬆️
exchanges/gateio/gateio_types.go 71.42% <ø> (+5.71%) ⬆️
exchanges/gemini/gemini.go 75.49% <100.00%> (+3.92%) ⬆️
exchanges/stream/websocket.go 82.53% <100.00%> (-0.11%) ⬇️
backtester/engine/setup.go 62.26% <0.00%> (+4.42%) ⬆️
... and 34 more

... and 350 files with indirect coverage 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, nice. Something I noticed if you want to change.

exchanges/binanceus/binanceus_types.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.

lint good
build good
updates good
I mistakenly was on golangci-lint v1.56.1 and made a diff for it, but oh well 😭

tACK!

@thrasher-
Copy link
Collaborator Author

lint good build good updates good I mistakenly was on golangci-lint v1.56.1 and made a diff for it, but oh well 😭

tACK!

Looks like they brought out 1.56.1 a day after this was done. Bumped it and applied the diff 🚀

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.

pedantic sorry.

engine/apiserver.go Outdated Show resolved Hide resolved
exchanges/exchange_test.go Outdated Show resolved Hide resolved
exchanges/exchange_test.go Outdated Show resolved Hide resolved
exchanges/request/request_test.go Outdated Show resolved Hide resolved
exchanges/request/request_test.go Outdated Show resolved Hide resolved
exchanges/request/request_test.go Outdated Show resolved Hide resolved
exchanges/stream/buffer/buffer_test.go Outdated Show resolved Hide resolved
exchanges/stream/buffer/buffer_test.go Outdated Show resolved Hide resolved
exchanges/stream/websocket_test.go Outdated Show resolved Hide resolved
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.

🚀 Nice ch ACK.

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! Excellent updates

@thrasher- thrasher- merged commit 08da42d into thrasher-corp:master Feb 14, 2024
8 of 12 checks passed
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