gitea/go-sdk
44
46
Fork 72
Code Issues 21 Pull Requests 7 Actions Releases 19 Activity

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
Conversation 6 Commits 13 Files Changed 29 +83 -101
Gusted commented 2022-04-27 19:09:04 +00:00
Contributor
Copy Link
  • 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
Enforce linter
Some checks failed
continuous-integration/drone/pr Build is failing
Details
bc98518169
Gusted added 1 commit 2022-04-27 19:10:09 +00:00
Empty commit
Some checks failed
continuous-integration/drone/pr Build is failing
Details
e2df5aae53
jolheiser reviewed 2022-04-27 19:14:21 +00:00
gitea/notifications.go Outdated
@ -15,3 +15,1 @@
var (
version1_12_3, _ = version.NewVersion("1.12.3")
)
var version1_12_3 = version.Must(version.NewVersion("1.12.3"))
jolheiser commented 2022-04-27 19:14:15 +00:00
Owner
Copy Link

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
gitea/org_team.go Outdated
@ -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) {
jolheiser commented 2022-04-27 19:12:23 +00:00
Owner
Copy Link

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?
Gusted commented 2022-04-27 19:17:10 +00:00
Author
Contributor
Copy Link

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.
jolheiser commented 2022-04-27 19:20:19 +00:00
Owner
Copy Link

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.
👍 1
Gusted commented 2022-04-27 19:22:37 +00:00
Author
Contributor
Copy Link

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.
jolheiser commented 2022-04-27 19:27:20 +00:00
Owner
Copy Link

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. ?
Gusted commented 2022-04-27 19:30:23 +00:00
Author
Contributor
Copy Link

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 ?.
😆 1
jolheiser marked this conversation as resolved
gitea/org_team.go Outdated
@ -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) {
jolheiser commented 2022-04-27 19:12:33 +00:00
Owner
Copy Link

Same question

Same question
Gusted marked this conversation as resolved
gitea/repo_collaborator.go Outdated
@ -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) {
jolheiser commented 2022-04-27 19:13:14 +00:00
Owner
Copy Link

Same question

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

Finally, CI failures relevant.

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

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

CI Isn't passing!! Please do not merge yet.
jolheiser commented 2022-04-28 15:08:59 +00:00
Owner
Copy Link

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
Downgrade golangci-lint
Some checks failed
continuous-integration/drone/pr Build is failing
Details
9aeb62b7b2
Gusted commented 2022-04-28 15:10:32 +00:00
Author
Contributor
Copy Link

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
Go back to go run ...
All checks were successful
continuous-integration/drone/pr Build is passing
Details
beb108d110
Gusted commented 2022-04-28 15:15:17 +00:00
Author
Contributor
Copy Link

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
👍 1 :gitea: 1
jolheiser commented 2022-04-28 15:16:24 +00:00
Owner
Copy Link

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.
👀 1
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.
Reviewers
No reviewers
jolheiser
lunny
Labels
Clear labels   
has/backport

   
has/pull

   
in progress

   
invalid

   
kind/breaking

   
kind/bug

   
kind/build

   
kind/deployment

   
kind/docs

   
kind/enhancement

   
kind/feature

   
kind/lint

   
kind/proposal

   
kind/question

   
kind/refactor

   
kind/security

   
kind/testing

   
kind/translation

   
kind/ui

   
need/backport

   
priority/critical

   
priority/low

   
priority/maybe

   
priority/medium

   
reviewed/duplicate

   
reviewed/invalid

   
reviewed/wontfix

   
skip-changelog

   
status/blocked

   
status/needs-feedback

   
status/needs-reviews

   
status/wip

   
upstream/gitea
No Label
has/backport
has/pull
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: gitea/go-sdk#587
No description provided.
Delete Branch "Gusted/go-sdk:fix-some-issues"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?