Enforce golangci-lint + gofumpt #587

Merged
lunny merged 13 commits from Gusted/go-sdk:fix-some-issues into master 2022-04-28 15:33:22 +00:00
Contributor
  • Enforce gofumpt to enforce a more idiomatic go style.
  • Enforce golangci-lint a bunch of linters! Which were able to detect a few issues in the current codebase and have been fixed by this PR.
  • Updated the Makefile to use go install .... instead of the old deprecated way of go get
- Enforce [gofumpt](https://github.com/mvdan/gofumpt) to enforce a more idiomatic go style. - Enforce golangci-lint a bunch of linters! Which were able to detect a few issues in the current codebase and have been fixed by this PR. - Updated the Makefile to use `go install ....` instead of the old deprecated way of `go get`
Gusted added 4 commits 2022-04-27 19:09:05 +00:00
Gusted added 1 commit 2022-04-27 19:10:09 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
e2df5aae53
Empty commit
jolheiser reviewed 2022-04-27 19:14:21 +00:00
@ -15,3 +15,1 @@
var (
version1_12_3, _ = version.NewVersion("1.12.3")
)
var version1_12_3 = version.Must(version.NewVersion("1.12.3"))
Owner

As an aside, we should probably just move this into version.go with the others.

As an aside, we should probably just move this into `version.go` with the others.
Gusted marked this conversation as resolved
@ -145,3 +145,3 @@
// CreateTeam creates a team for an organization
func (c *Client) CreateTeam(org string, opt CreateTeamOption) (*Team, *Response, error) {
func (c *Client) CreateTeam(org string, opt *CreateTeamOption) (*Team, *Response, error) {
Owner

Why do we want this to be a pointer? Is nil a valid thing to pass?

Why do we want this to be a pointer? Is `nil` a valid thing to pass?
Author
Contributor

Easier to work on, see the changes in validate, otherwise we will end up (&opt).Validate() which is hacky IMO. But happy to revert it.

Easier to work on, see the changes in validate, otherwise we will end up `(&opt).Validate()` which is hacky IMO. But happy to revert it.
Owner

Oof, I'm not (personally) a fan of side-effects like that.
I would think defaults should be set prior to Validate rather than Validate changing the struct.

I feel like passing in a pointer just opens a footgun whereby passing nil could cause a panic.

Oof, I'm not (personally) a fan of side-effects like that. I would think defaults should be set prior to `Validate` rather than `Validate` changing the struct. I feel like passing in a pointer just opens a footgun whereby passing `nil` could cause a panic.
Author
Contributor

They aren't defaults, it's seems to "fix" certain options(e.g. opt.Permission AccessModeOwner -> AccessModeAdmin), such that it would be accepted by the Gitea server.

They aren't defaults, it's seems to "fix" certain options(e.g. `opt.Permission` AccessModeOwner -> AccessModeAdmin), such that it would be accepted by the Gitea server.
Owner

Hmm...I'm not convinced that Validate should modify the struct, though that's largely personal preference.

I also agree (&opt).Validate() looks a bit silly, but from an API standpoint we may as well keep guns away from feet. ?

Hmm...I'm not convinced that `Validate` should modify the struct, though that's largely personal preference. I also agree `(&opt).Validate()` looks a bit silly, but from an API standpoint we may as well keep guns away from feet. ?
Author
Contributor

Don't tell anyone, but this behavior was already here... But just broken ?.

Don't tell anyone, but this behavior was already here... But just broken ?.
jolheiser marked this conversation as resolved
@ -191,3 +191,3 @@
// EditTeam edits a team of an organization
func (c *Client) EditTeam(id int64, opt EditTeamOption) (*Response, error) {
func (c *Client) EditTeam(id int64, opt *EditTeamOption) (*Response, error) {
Owner

Same question

Same question
Gusted marked this conversation as resolved
@ -85,3 +85,3 @@
// AddCollaborator add some user as a collaborator of a repository
func (c *Client) AddCollaborator(user, repo, collaborator string, opt AddCollaboratorOption) (*Response, error) {
func (c *Client) AddCollaborator(user, repo, collaborator string, opt *AddCollaboratorOption) (*Response, error) {
Owner

Same question

Same question
Gusted marked this conversation as resolved
Gusted added 2 commits 2022-04-27 19:20:43 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
badd226733
Pass direct pointer of given variable
Owner

Finally, CI failures relevant.

Finally, CI failures relevant.
Gusted added 1 commit 2022-04-28 15:00:32 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
c27e8bdac6
Fix CI
Gusted added 1 commit 2022-04-28 15:03:28 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
3d82a27048
Merge branch 'master' into fix-some-issues
jolheiser approved these changes 2022-04-28 15:06:03 +00:00
Gusted added 1 commit 2022-04-28 15:06:40 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
3b52b94169
Bump go version in CI
Author
Contributor

CI Isn't passing!! Please do not merge yet.

CI Isn't passing!! Please do not merge yet.
Owner

Odd, it was passing. Looks like a sum error now?

Odd, it _was_ passing. Looks like a sum error now?
Gusted added 1 commit 2022-04-28 15:09:32 +00:00
Some checks failed
continuous-integration/drone/pr Build is failing
9aeb62b7b2
Downgrade golangci-lint
Author
Contributor

Odd, it was passing. Looks like a sum error now?

It was a lie, the binaries weren't downloaded(see the ... not found errors). And now it's downloading and it's giving a sum error 😄 This will be a interesting rabbit hole.

> Odd, it _was_ passing. Looks like a sum error now? It was a lie, the binaries weren't downloaded(see the `... not found` errors). And now it's downloading and it's giving a sum error :smile: This will be a interesting rabbit hole.
Gusted added 2 commits 2022-04-28 15:11:55 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
beb108d110
Go back to go run ...
Author
Contributor

So for some reason the latest golangci-lint version has errors with go sum(probably some linter changed something) so we're running an older version, the same Gitea currently uses.

The CI now also pulls go1.18, because go1.17+ allows to use go run package_here without having to use go install ... or go get -u ... first. And now the CI is passing and actually linting! So we can safely merge :D

So for some reason the latest golangci-lint version has errors with go sum(probably some linter changed something) so we're running an older version, the same Gitea currently uses. The CI now also pulls go1.18, because go1.17+ allows to use `go run package_here` without having to use `go install ...` or `go get -u ...` first. And now the CI is passing and actually linting! So we can safely merge :D
Owner

Odd, it was passing. Looks like a sum error now?

It was a lie, the binaries weren't downloaded(see the ... not found errors). And now it's downloading and it's giving a sum error 😄 This will be a interesting rabbit hole.

Oho! Sneaky CI!
Oof, yeah that might be fun.
Perhaps CI should use images for our linters? That way it doesn't need to build them every time.

> > Odd, it _was_ passing. Looks like a sum error now? > > It was a lie, the binaries weren't downloaded(see the `... not found` errors). And now it's downloading and it's giving a sum error :smile: This will be a interesting rabbit hole. Oho! Sneaky CI! Oof, yeah that might be fun. Perhaps CI should use images for our linters? That way it doesn't need to build them every time.
lunny approved these changes 2022-04-28 15:33:10 +00:00
lunny merged commit 8fab37e740 into master 2022-04-28 15:33:22 +00:00
Sign in to join this conversation.
No description provided.