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.

Sign up for GitHub

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

Fix make fmt and make fmt-check #18633

Merged
merged 11 commits into from
Feb 6, 2022
Merged

Fix make fmt and make fmt-check #18633

merged 11 commits into from
Feb 6, 2022

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 6, 2022 •
edited
Loading

'make fmt' currently produces this change, I'm not sure how CI did not fail on it, I made sure I have mvdan.cc/gofumpt@latest.

cc: @6543

@silverwind
Run 'make fmt'
9602a9b 
'make fmt' currently produces this change, I'm not sure how CI did not
fail on it, I made sure I have `mvdan.cc/gofumpt@latest`.
@silverwind silverwind added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Feb 6, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Feb 6, 2022
@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 Feb 6, 2022
@silverwind
Copy link
Member Author

Replaced by #18634 which includes this change.

@silverwind silverwind closed this Feb 6, 2022
@silverwind silverwind deleted the fmt-file branch February 6, 2022 07:13
@silverwind silverwind mentioned this pull request Feb 6, 2022
Closed
@silverwind silverwind restored the fmt-file branch February 6, 2022 07:16
@silverwind
Copy link
Member Author

Should still land this as I'm not totally sure whether #18634 is correct.

@silverwind silverwind reopened this Feb 6, 2022
lunny and others added 5 commits February 6, 2022 15:19
@lunny
Merge branch 'main' into fmt-file
ac65808
@silverwind
Fix 'make fmt-check'
5b924d9 
`make fmt-check` did not run all commands that `make fmt` did, resulting
in missed diffs. Fix that by just depending on the `fmt` target.

Includes: go-gitea#18633
@zeripath
Merge remote-tracking branch 'origin/main' into fix-fmt
b049659
@zeripath
Make gitea-fmt work with -l and -d and integrate gofumpt
32b84e0 
This implements -l, -w and -d with gitea-fmt and merges gofumpt.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Merge branch 'fix-fmt' into fmt-file
69fc636
@zeripath zeripath changed the title Run 'make fmt' Fix make fmt and make fmt-check Feb 6, 2022
@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

The problem is twofold:

  1. The gofumpt PR did not properly integrate gofumpt into the already existing fmt pipeline. Instead of calling gofmt in code-batch-process it should call gofumpt. (And drop the -s argument as it is no longer needed.)
  2. The gitea-fmt command needs to properly handle -l and -d arguments.

Therefore we need to make the gitea-format code emit a diff on -d argument

@zeripath
as per silverwind
c770330 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Merge branch 'main' into fmt-file
203dfe2
zeripath
zeripath reviewed Feb 6, 2022
View reviewed changes
build/code-batch-process.go Outdated Show resolved Hide resolved
build/code-batch-process.go Outdated Show resolved Hide resolved
wxiaoguang
wxiaoguang requested changes Feb 6, 2022
View reviewed changes
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment •
edited
Loading

Choose a reason for hiding this comment

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

In my mind we do not need github.com/hexops/gotextdiff, fmt-check (gitea-fmt) could only report whether the file is correctly formatted (or show the incorrect file list), it doesn't need to report the diff.

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

In my mind we do not need github.com/hexops/gotextdiff, fmt-check (gitea-fmt) could only report whether the file is correctly formatted (or show the incorrect file list), it doesn't need to report the diff.

Fine.

We can always add the diff format back in if we need it.

@zeripath
use -l instead of -d for fmt-check
5f951f8 
Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543 6543 merged commit 37ead26 into go-gitea:main Feb 6, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 8, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
06817b1 
* giteaofficial/main: (28 commits)
  Added auto-save whitespace behavior if it changed manually (go-gitea#15566)
  Support custom ACME provider (go-gitea#18340)
  Refactor i18n, use Locale to provide i18n/translation related functions (go-gitea#18648)
  Only request write when necessary (go-gitea#18657)
  [skip ci] Updated translations via Crowdin
  Add separate SSH_USER config option (go-gitea#17584)
  Be more lenient with label colors (go-gitea#17752)
  remove redundant call to UpdateRepoStats during migration (go-gitea#18591)
  more repo dump/restore tests, including pull requests (go-gitea#18621)
  No longer show the db-downgrade SQL in production (go-gitea#18653)
  Fix the missing i18n key for update checker (go-gitea#18646)
  Update gitea-vet (go-gitea#18640)
  Future proof for 1.18 (go-gitea#18644)
  Add `contrib/upgrade.sh` (go-gitea#18286)
  If rendering has failed due to a net.OpError stop rendering (go-gitea#18642)
  Delete old git.NewCommand() and use it as git.NewCommandContext() (go-gitea#18552)
  Update JS dependencies (go-gitea#18636)
  fix commits_list_small.tmpl (go-gitea#18641)
  Fix `make fmt` and `make fmt-check` (go-gitea#18633)
  Frontport of changelog for v1.16.1 (go-gitea#18615)
  ...
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@silverwind @lunny @zeripath
Fix make fmt and make fmt-check (go-gitea#18633)
99c1882 
* Run 'make fmt'

'make fmt' currently produces this change, I'm not sure how CI did not
fail on it, I made sure I have `mvdan.cc/gofumpt@latest`.

* Fix 'make fmt-check'

`make fmt-check` did not run all commands that `make fmt` did, resulting
in missed diffs. Fix that by just depending on the `fmt` target.

Includes: go-gitea#18633

* Make gitea-fmt work with -l and -d and integrate gofumpt

This implements -l, -w and -d with gitea-fmt and merges gofumpt.

Signed-off-by: Andrew Thornton <art27@cantab.net>

* as per silverwind

Signed-off-by: Andrew Thornton <art27@cantab.net>

* Apply suggestions from code review

* use -l instead of -d for fmt-check

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: Andrew Thornton <art27@cantab.net>
@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

@zeripath zeripath zeripath approved these changes

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@lunny lunny lunny approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@silverwind @zeripath @lunny @wxiaoguang @GiteaBot @6543