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: Add golangci-lint #690

Merged
merged 3 commits into from
Jul 26, 2021
Merged

CI: Add golangci-lint #690

merged 3 commits into from
Jul 26, 2021

Conversation

burntcarrot
Copy link
Member

Signed-off-by: burntcarrot aadhav.n1@gmail.com

Resolves #689.

Signed-off-by: burntcarrot <aadhav.n1@gmail.com>
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #690 (51ad739) into master (d145186) will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #690      +/-   ##
==========================================
+ Coverage   12.39%   12.58%   +0.19%     
==========================================
  Files          22       22              
  Lines        1436     1414      -22     
==========================================
  Hits          178      178              
+ Misses       1251     1229      -22     
  Partials        7        7              
Flag Coverage Δ
unittests 12.58% <ø> (+0.19%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/service.go 0.00% <0.00%> (ø)
tests/storage.go 0.00% <0.00%> (ø)

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 d145186...51ad739. Read the comment docs.

@burntcarrot
Copy link
Member Author

burntcarrot commented Jul 23, 2021

@Xuanwo Lint is working as expected. Would you like to open an issue related to the unused variables found in the golangci-lint check, or do you want me to comment out the unused variables?

@burntcarrot
Copy link
Member Author

Since the workflow doesn't show a complete traceback, I decided to run it on my local machine:

pkg/iowrap/pipe.go:41:2: `rerr` is unused (structcheck)
        rerr error
        ^
pkg/iowrap/pipe.go:40:2: `werr` is unused (structcheck)
        werr error
        ^
tests/service.go:11:2: `features` is unused (structcheck)
        features     ServiceFeatures
        ^
pkg/iowrap/pipe.go:112:2: S1023: redundant `return` statement (gosimple)
        return
        ^
pkg/iowrap/pipe.go:172:2: S1023: redundant `return` statement (gosimple)
        return
        ^
pkg/fswrap/httpfs.go:46:2: SA4005: ineffective assignment to field httpFileWrapper.store (staticcheck)
        h.store = nil
        ^
pkg/fswrap/httpfs.go:47:2: SA4005: ineffective assignment to field httpFileWrapper.object (staticcheck)
        h.object = nil
        ^
pkg/fswrap/httpfs.go:48:2: SA4005: ineffective assignment to field httpFileWrapper.offset (staticcheck)
        h.offset = 0
        ^
pkg/fswrap/iofs.go:147:2: SA4005: ineffective assignment to field fileWrapper.offset (staticcheck)
        o.offset = 0
        ^
pkg/fswrap/iofs.go:145:2: SA4005: ineffective assignment to field fileWrapper.store (staticcheck)
        o.store = nil
        ^
pkg/fswrap/iofs.go:146:2: SA4005: ineffective assignment to field fileWrapper.object (staticcheck)
        o.object = nil
        ^

Hope this helps!

@Xuanwo
Copy link
Contributor

Xuanwo commented Jul 24, 2021

pkg/iowrap/pipe.go:41:2: rerr is unused (structcheck)
rerr error
^

OK to remove.

pkg/iowrap/pipe.go:40:2: werr is unused (structcheck)
werr error
^

OK to remove.

tests/service.go:11:2: features is unused (structcheck)
features ServiceFeatures
^

Use it by calling parsePairServiceNew and assign it like https://github.com/beyondstorage/go-service-s3/blob/master/utils.go#L158-L159.

pkg/iowrap/pipe.go:112:2: S1023: redundant return statement (gosimple)
return
^

OK to remove.

pkg/iowrap/pipe.go:172:2: S1023: redundant return statement (gosimple)
return
^

OK to remove.

pkg/fswrap/httpfs.go:46:2: SA4005: ineffective assignment to field httpFileWrapper.store (staticcheck)
h.store = nil
^

OK to remove.

pkg/fswrap/httpfs.go:47:2: SA4005: ineffective assignment to field httpFileWrapper.object (staticcheck)
h.object = nil
^

OK to remove.

pkg/fswrap/httpfs.go:48:2: SA4005: ineffective assignment to field httpFileWrapper.offset (staticcheck)
h.offset = 0
^

OK to remove.

pkg/fswrap/iofs.go:147:2: SA4005: ineffective assignment to field fileWrapper.offset (staticcheck)
o.offset = 0
^

OK to remove.

pkg/fswrap/iofs.go:145:2: SA4005: ineffective assignment to field fileWrapper.store (staticcheck)
o.store = nil
^

OK to remove.

pkg/fswrap/iofs.go:146:2: SA4005: ineffective assignment to field fileWrapper.object (staticcheck)
o.object = nil
^

OK to remove.


All of them are easy to solve, would you be willing to open a new PR to fix them?

GitHub
Amazon S3 support for go-storage. Contribute to beyondstorage/go-service-s3 development by creating an account on GitHub.

@burntcarrot
Copy link
Member Author

burntcarrot commented Jul 24, 2021

Yes, I'll be happy to make a new PR.

I don't think removing werr and rerr would be a good choice, because if they might be implemented in the future, deleting them now will create a mess. I propose that we comment those fields out and add a TODO note instead.

I'll remove the rest of the ineffectual assignments and redundant return statements in a new PR.

@Xuanwo Xuanwo changed the title feat: add golangci-lint CI: Add golangci-lint Jul 25, 2021
@@ -0,0 +1,37 @@
name: golangci-lint
on:
Copy link
Contributor

Choose a reason for hiding this comment

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

How about executing golangco-lint on [push, pull_request]?

- name: golangci-lint
uses: golangci/golangci-lint-action@v2
with:
# Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version
Copy link
Contributor

Choose a reason for hiding this comment

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

How about removing all useless comments?

- master
pull_request:
jobs:
golangci:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
golangci:
golangci_lint:

@burntcarrot
Copy link
Member Author

Can you please implement those changes as proposed above? My editor (VSCode) has been checking out to the wrong branch and due to this I've been making improper commits.

Can you also please remove the action from PR 691 if possible. I'll try to fix my development environment, apologies for the fuss created.

@Xuanwo Xuanwo merged commit 29366c6 into beyondstorage:master Jul 26, 2021
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.

Setup golangci-lint for all golang projects
2 participants