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

Remove broken and useless file in build #20952

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Remove broken and useless file in build #20952

merged 1 commit into from
Aug 25, 2022

Conversation

wolfogre
Copy link
Member

@wolfogre wolfogre commented Aug 25, 2022

Related to #17684, #18633.

After the changing of codeformat.FormatGoImports, the gitea-format-imports.go became uncompilable.

And According to the comment:

// We also introduce a `gitea-fmt` command, it does better import formatting than gofmt/goimports

I think it would be clearer to rename gitea-format-imports.go to gitea-fmt.go

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Aug 25, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 25, 2022

+1.

FYI, that's a historical reason of the name "code-batch-process.go".

At that time, Windows developers can not run make fmt because Windows can not accept long argument strings, the command ./program arg1 arg2 ... a-lot-of-args always fails on Windows. That's why code-batch-process.go comes, it splits the command line arguments into groups and run separate commands with fewer arguments: ./program arg1 arg2 ... argN and ./program argN+1 argN+2 ... argN+N, and so on.

After many code changes, Gitea has dropped many old tools, and I think most modern tools do not need to use ./program arg1 arg2 ... a-lot-of-args to pass filenames and won't have the annoying Windows command argument problem.

If it's the case, maybe it's time to retire the argument splitting function of code-batch-process.go

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 25, 2022
@wolfogre
Copy link
Member Author

@wxiaoguang You may may have misunderstood me, I mean rename gitea-format-imports.go to gitea-fmt.go.

wxiaoguang
wxiaoguang previously approved these changes Aug 25, 2022
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.

Hmm, I saw code-batch-process.go in the PR comment so added more information about it (at the beginning, they are related ...), and maybe other improvements can be done in the future. (I hidden the previous comment as off-topic)

@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 Aug 25, 2022
Makefile Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 25, 2022

Although the change L-G-T-M, I am not sure the gitea-fmt is really a good name for it, since it only handles imports now.

ps: there is also gitea-vet (https://gitea.com/gitea/gitea-vet) 😁 maybe it's time to improve some toolchains for Gitea together.

Since I have no preference and have no objection, I will put my L-G-T-M here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 25, 2022
@wxiaoguang wxiaoguang removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Aug 25, 2022
@wolfogre
Copy link
Member Author

Or we just remove the broken gitea-format-imports.go? It is not used by Makefile, and the command gitea-fmt could be replaced by gitea-vet.

@wxiaoguang
Copy link
Contributor

Or we just remove the broken gitea-format-imports.go? It is not used by Makefile, and the command gitea-fmt could be replaced by gitea-vet.

Agree, removing gitea-format-imports.go seems a clear solution. And in the future, gitea-vet could take more works (if most developers agree)

@wolfogre wolfogre changed the title Fix command gitea-fmt Remove broken and useless file in build Aug 25, 2022
@wolfogre
Copy link
Member Author

Changed the commits, just removed the broken file and the outdated introduction which confused me 😂

@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 Aug 25, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 25, 2022

Changed the commits, just removed the broken file and the outdated introduction which confused me 😂

Actually, the comment is correct (but not clear ....)

switch subCmd {
case "gitea-fmt":
if containsString(subArgs, "-d") {
log.Print("the -d option is not supported by gitea-fmt")
}
cmdErrors = append(cmdErrors, giteaFormatGoImports(files, containsString(subArgs, "-l"), containsString(subArgs, "-w")))
cmdErrors = append(cmdErrors, passThroughCmd("go", append([]string{"run", os.Getenv("GOFUMPT_PACKAGE"), "-extra", "-lang", "1.17"}, substArgs...)))

@wolfogre
Copy link
Member Author

Changed the commits, just removed the broken file and the outdated introduction which confused me 😂

Actually, the comment is correct (but not clear ....), I made some changes to it.

switch subCmd {
case "gitea-fmt":
if containsString(subArgs, "-d") {
log.Print("the -d option is not supported by gitea-fmt")
}
cmdErrors = append(cmdErrors, giteaFormatGoImports(files, containsString(subArgs, "-l"), containsString(subArgs, "-w")))
cmdErrors = append(cmdErrors, passThroughCmd("go", append([]string{"run", os.Getenv("GOFUMPT_PACKAGE"), "-extra", "-lang", "1.17"}, substArgs...)))

I see, I dropped the comment deleting.

@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 Aug 25, 2022
@wxiaoguang wxiaoguang merged commit 9e3aa4d into go-gitea:main Aug 25, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Aug 26, 2022
* upstream/main:
  Add whitespace removal inside template curly brackes (go-gitea#20853)
  Only show relevant repositories on explore page (go-gitea#19361)
  Replace `ServeStream` with `ServeContent` (go-gitea#20903)
  Update JS dependencies (go-gitea#20950)
  chore: remove broken gitea-format-imports (go-gitea#20952)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 28, 2022
@wxiaoguang wxiaoguang added this to the 1.18.0 milestone Oct 7, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants