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

Jump to bottom

format with gofumpt #18184

Merged
merged 7 commits into from Jan 20, 2022
Merged

format with gofumpt #18184

merged 7 commits into from Jan 20, 2022

Conversation

6543
Copy link
Member

@6543 6543 commented Jan 4, 2022

as title

6543
6543 commented Jan 4, 2022
View reviewed changes
services/migrations/gogs_test.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 4, 2022
6543
6543 commented Jan 4, 2022
View reviewed changes
services/mailer/mailer_test.go Outdated Show resolved Hide resolved
6543
6543 commented Jan 4, 2022
View reviewed changes
routers/web/web.go Outdated Show resolved Hide resolved
6543
6543 commented Jan 4, 2022
View reviewed changes
services/migrations/gogs_test.go Outdated Show resolved Hide resolved
services/migrations/gogs_test.go Outdated Show resolved Hide resolved
services/migrations/gitlab_test.go Outdated Show resolved Hide resolved
services/migrations/gitlab_test.go Outdated Show resolved Hide resolved
services/migrations/gitlab_test.go Outdated Show resolved Hide resolved
services/migrations/github_test.go Outdated Show resolved Hide resolved
services/migrations/github_test.go Outdated Show resolved Hide resolved
services/migrations/github_test.go Outdated Show resolved Hide resolved
services/migrations/github_test.go Outdated Show resolved Hide resolved
services/migrations/github_test.go Outdated Show resolved Hide resolved
@6543 6543 marked this pull request as ready for review January 4, 2022 20:31
@6543 6543 added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 4, 2022
Gusted
Gusted reviewed Jan 4, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Just a lot of off-topic not related things.

integrations/api_user_orgs_test.go
Comment on lines +57 to 60
token := ""
session := emptyTestSession(t)
if len(userDoer) != 0 {
session = loginUser(t, userDoer)
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
token := ""
session := emptyTestSession(t)
if len(userDoer) != 0 {
session = loginUser(t, userDoer)
session := emptyTestSession(t)
if len(userDoer) != 0 {
var token string
session = loginUser(t, userDoer)

integrations/git_smart_http_test.go
@@ -18,7 +18,7 @@ func TestGitSmartHTTP(t *testing.T) {
}

func testGitSmartHTTP(t *testing.T, u *url.URL) {
var kases = []struct {
kases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo 😄

modules/context/api_test.go
@@ -16,7 +16,7 @@ import (

func TestGenAPILinks(t *testing.T) {
setting.AppURL = "http://localhost:3000/"
var kases = map[string][]string{
kases := map[string][]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

modules/git/submodule_test.go
@@ -11,7 +11,7 @@ import (
)

func TestGetRefURL(t *testing.T) {
var kases = []struct {
kases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again... typo

modules/log/file.go
@@ -76,7 +76,7 @@ func (mw *MuxWriter) SetFd(fd *os.File) {
func NewFileLogger() LoggerProvider {
log := &FileLogger{
Filename: "",
Maxsize: 1 << 28, //256 MB
Maxsize: 1 << 28, // 256 MB
Copy link
Contributor

Choose a reason for hiding this comment

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

Just off-topic, but now I see this anyway 1 << 28 is 268MB. Maybe 256e6 should be used, which is 256MB.

modules/public/public_test.go
@@ -11,7 +11,7 @@ import (
)

func TestParseAcceptEncoding(t *testing.T) {
var kases = []struct {
kases := []struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

... I will personally send a new c key,

routers/web/admin/admin_test.go Show resolved Hide resolved
routers/web/repo/editor_test.go Show resolved Hide resolved
routers/web/repo/issue_test.go Show resolved Hide resolved
services/webhook/deliver_test.go Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Jan 4, 2022

@Gusted I already have one off-topic in this pull - I like to have your suggestions in an extra pull - this diff is already huge!!!

Gusted
Gusted approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

Voila

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 4, 2022
@6543 6543 added this to the 1.17.0 milestone Jan 4, 2022
wxiaoguang
wxiaoguang approved these changes Jan 5, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

gofumpt really does a good job

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 5, 2022
@wxiaoguang
Copy link
Contributor

ps: @Gusted IMO the kase is not a typo but choose by purpose, to bypass the keyword case, just like many Java developers like to write klass for a variable name to bypass the class keyword.

@6543 6543 mentioned this pull request Jan 5, 2022
Merged
@Gusted
Copy link
Contributor

Gusted commented Jan 5, 2022

ps: @Gusted IMO the kase is not a typo but choose by purpose, to bypass the keyword case, just like many Java developers like to write klass for a variable name to bypass the class keyword.

Ah I see, thanks! It was so confusing to see it being used.

@wxiaoguang
Copy link
Contributor

One more thing, this PR could replace an old PR

@6543
Copy link
Member Author

6543 commented Jan 6, 2022

I'll merge once we have an release branch

@wxiaoguang
Copy link
Contributor

I'll merge once we have an release branch

I prefer to get it merged in 1.16.

Reasons:

  1. This is only a refactoring, doesn't change the logic.
  2. Merging it now will avoid potential conflicts in future.
  3. There are still some open PRs for 1.16 and it still gets fixes.

zeripath
zeripath previously requested changes Jan 7, 2022
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment •
edited

Choose a reason for hiding this comment

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

you need to update:

make fmt
make fmt-check

We need gofumpt to be autodownloaded too.

And we probably also need to update the Dev documentation to explain you need gofumpt in vscode goland with explanations on how to enable that.

@wxiaoguang wxiaoguang mentioned this pull request Jan 20, 2022
Closed
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jan 20, 2022 •
edited

Conflicts come again .... I was thinking about we should get this merged as early as possible in 1.16 .....

Conflicts are dangerous, a lot of bugs are caused by conflicts.

@zeripath
Copy link
Contributor

Agreed - but we need the above makefile things to be fixed.

Now fortunately I think just rerunning gofumpt on the code will make them go away.

(That of course will not necessarily work for conflicts that merging this PR will cause with other PRs.)

@6543 6543 requested a review from zeripath January 20, 2022 17:06
@6543 6543 removed the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 20, 2022
@zeripath zeripath dismissed their stale review January 20, 2022 17:20

changes done

@6543 6543 merged commit 54e9ee3 into go-gitea:main Jan 20, 2022
@6543 6543 deleted the gofumpt branch January 20, 2022 17:46
zjjhot pushed a commit to zjjhot/gitea that referenced this pull request Jan 21, 2022
Merge remote-tracking branch 'giteaoffical/main'
0e80795 
* giteaoffical/main:
  [skip ci] Updated translations via Crowdin
  Refactor jwt.StandardClaims to RegisteredClaims (go-gitea#18344)
  format with gofumpt (go-gitea#18184)
  Enable deprecation error for v1.17.0 (go-gitea#18341)
  Use correct translation key for errors (go-gitea#18342)
  Refactor Router Logger (go-gitea#17308)
  Updated Chroma to v0.10.0 (go-gitea#18270)
@wxiaoguang
Copy link
Contributor

@6543 @zeripath

I think we still need @$(GO) run build/code-batch-process.go {something} -s -w '{file-list}' (code.gitea.io/gitea/build/codeformat)

It can group the imports.

The output o f gofumpt could be:

import (
	"os"
	"path"

	"code.gitea.io/gitea/modules/log"  // gofumpt doesn't group imports

	"code.gitea.io/gitea/modules/httpcache"
	"code.gitea.io/gitea/modules/setting"
)

Our code.gitea.io/gitea/build/codeformat can format the code above to:

import (
	"os"
	"path"

	"code.gitea.io/gitea/modules/httpcache"
	"code.gitea.io/gitea/modules/log"
	"code.gitea.io/gitea/modules/setting"
)

@lunny
Copy link
Member

lunny commented Jan 21, 2022

@6543 @zeripath

I think we still need @$(GO) run build/code-batch-process.go {something} -s -w '{file-list}' (code.gitea.io/gitea/build/codeformat)

It can group the imports.

The output o f gofumpt could be:

import (
	"os"
	"path"

	"code.gitea.io/gitea/modules/log"  // gofumpt doesn't group imports

	"code.gitea.io/gitea/modules/httpcache"
	"code.gitea.io/gitea/modules/setting"
)

Our code.gitea.io/gitea/build/codeformat can format the code above to:

import (
	"os"
	"path"

	"code.gitea.io/gitea/modules/httpcache"
	"code.gitea.io/gitea/modules/log"
	"code.gitea.io/gitea/modules/setting"
)

I think so.

@6543
Copy link
Member Author

6543 commented Jan 21, 2022

ok will do a follow up just for the make fmt

@6543 6543 mentioned this pull request Feb 1, 2022
Merged
@6543
Copy link
Member Author

6543 commented Feb 1, 2022

-> #18526

Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@6543
format with gofumpt (go-gitea#18184)
5332096 
* gofumpt -w -l .

* gofumpt -w -l -extra .

* Add linter

* manual fix

* change make fmt
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jolheiser jolheiser jolheiser left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@Gusted Gusted Gusted approved these changes

@zeripath zeripath Awaiting requested review from zeripath

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@6543 @wxiaoguang @Gusted @zeripath @lunny @jolheiser @GiteaBot