Skip to content

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Feb 6, 2022

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: #18633

@silverwind silverwind added topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Feb 6, 2022
@silverwind
Copy link
Member Author

silverwind commented Feb 6, 2022

There is one difference in the arguments, gitea-fmt took -d in fmt-check while fmt took -w, so I'm not sure whether this fix is correct. cc: @6543.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 6, 2022
@silverwind silverwind changed the title Fix 'make fmt' Fix 'make fmt-check' Feb 6, 2022
`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
@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 Feb 6, 2022
@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

https://pkg.go.dev/cmd/gofmt

-d
	Do not print reformatted sources to standard output.
	If a file's formatting is different than gofmt's, print diffs
	to standard output.

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

The problem is that gitea-fmt doesn't properly support the -d as far as I can see

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

Signed-off-by: Andrew Thornton <[email protected]>
@silverwind
Copy link
Member Author

silverwind commented Feb 6, 2022

I think my version was fine. We want it to just alter the files in-place in both cases. No need for duplicate code that risk going out of sync on careless edits.

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

I'm gonna merge this in to your other PR.

@zeripath zeripath closed this Feb 6, 2022
@silverwind
Copy link
Member Author

Can you make it so fmt-check still depends on fmt?

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

Nope they're calling different args - one is doing diff and one is writing files.

@silverwind
Copy link
Member Author

Seems a bit pointless to not just rely on git diff :)

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

then fmt-check is dependent on the the status of your git index which is very non-ideal.

@silverwind
Copy link
Member Author

Does it matter when the target realistically is only triggered on CI where index is clean?

@zeripath
Copy link
Contributor

zeripath commented Feb 6, 2022

It is not only triggered on CI.

@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.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. 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.

3 participants