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

Use own Go version instead of hardcoded 1.17 for make fmt #21457

Merged
merged 4 commits into from Oct 15, 2022

Conversation

yardenshoham
Copy link
Member

We should make sure we're using the same version across the codebase.

@silverwind
Copy link
Member

silverwind commented Oct 14, 2022

This whole code-batch-process.go thing seems rather insane to me. I'd rather have us run gofumpt directly which will work much better in regards to editor integrations and such.

The canonical way to specify go version to gofumpt is through -lang, so I would replicate that. SeeMIN_GO_VERSION in the Makefile.

go run mvdan.cc/gofumpt@latest --help |& grep lang
  -lang       str    target Go version in the form "1.X" (default from go.mod)

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 14, 2022
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Oct 14, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Oct 15, 2022

I do not like the code-batch-process.go either, but at that time it's needed for Windows developers to run make fmt (#14438)

Not sure whether the situation (eco-system) has been better now, if the code-batch-process.go is unnecessary any more, remove it.

@6543
Copy link
Member

6543 commented Oct 15, 2022

well i dont understand why one would programm on windows, but there are always the one who are forced to do so ... :/

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.

It seems that the only need for the code-batch-process is misspell, which still takes a file list as arguments.

I think we can get this PR at the moment and refactor the building system later, remove out-dated tools.


Update:

Actually, after

The misspell is never used, the only sub-command call to code-batch-format is gitea-fmt, no misspell anymore.

build/code-batch-process.go Outdated Show resolved Hide resolved
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.

The change looks good to me.

But I have a feeling that the code-batch-process will be removed soon and the Makefile will call the gitea-fmt directly without this wrapper.

@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 Oct 15, 2022
@yardenshoham
Copy link
Member Author

Sounds good to me, until then we can land thia

@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 Oct 15, 2022
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
Signed-off-by: Yarden Shoham <hrsi88@gmail.com>
@6543
Copy link
Member

6543 commented Oct 15, 2022

🚀

@zeripath
Copy link
Contributor

It seems that the only need for the code-batch-process is misspell, which still takes a file list as arguments.

It also runs the gitea import formatter.

@wxiaoguang
Copy link
Contributor

It seems that the only need for the code-batch-process is misspell, which still takes a file list as arguments.

It also runs the gitea import formatter.

Yup, I meant "which still takes a file list as arguments.", only the old misspell command needs it.

For gitea import formatter, it could be simplified to scan the source directory directly, no need for the code-batch-process wrapper anymore.

I think it's time to drop the code-batch-process wrapper (I will take the work)

@zeripath zeripath merged commit bc53256 into go-gitea:main Oct 15, 2022
@yardenshoham yardenshoham deleted the version branch October 15, 2022 20:51
@wxiaoguang
Copy link
Contributor

--> Remove code-batch-process #21471

@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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants