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

fmt cmd: use a tab instead of two spaces to format comments #1386

Merged
merged 3 commits into from
Nov 18, 2022
Merged

Conversation

sdghchj
Copy link
Member

@sdghchj sdghchj commented Nov 18, 2022

Describe the PR
change fmt cmd: use a tab instead of two spaces at the front of a comment line, to be compatibale with gofmt v1.19.

Relation issue
#1381

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Base: 95.71% // Head: 95.71% // No change to project coverage 👍

Coverage data is based on head (606c4cb) compared to base (eaed517).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1386   +/-   ##
=======================================
  Coverage   95.71%   95.71%           
=======================================
  Files          14       14           
  Lines        2826     2826           
=======================================
  Hits         2705     2705           
  Misses         66       66           
  Partials       55       55           
Impacted Files Coverage Δ
formatter.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sdghchj sdghchj changed the title fmt cmd: use a tab instead of two spaces at the front of a comment line fmt cmd: use a tab instead of two spaces to format comments Nov 18, 2022
Copy link
Contributor

@ubogdan ubogdan left a comment

Choose a reason for hiding this comment

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

LGTM

@sdghchj sdghchj merged commit 829fbe1 into master Nov 18, 2022
@sdghchj sdghchj deleted the fmt-gofmt branch November 18, 2022 18:00
@Zxilly
Copy link
Contributor

Zxilly commented Nov 20, 2022

Maybe should update the prefix space to tab but keep use space afterwards.

@morremeyer
Copy link
Contributor

With this change, swag init and gofumpt are incompatible since gofumpt does format comments to have one space between the start of the comment and the content.

I think using one space is better here since most people have their editors and IDEs configured to display tabs as multiples of 2 spaces, which looks off for 2 and - in my opinion - wastes screen real estate if we format comments with a tab displayed as 4 or 8 spaces.

While go does use tabs for code indentation, one space after the // seems to be much more common for comments.

I would therefore like to either revert this change or ideally have it configurable to use either a tab or a space.

What do you think?

Relates to #1381 and #1397.

@sdghchj
Copy link
Member Author

sdghchj commented Nov 25, 2022

@morremeyer
As I see, gofmt itself behaves differently with different versions, not to mention the supperset gofumpt.
go 1.6 does nothing for coments,
however, go 1.9 only handle the first line comment on a func by adding a space, but add a tab for remain lines, just like:

// GetStringByInt example
//
//	@Summary		Add a new pet to the store
//	@Description		get string by ID
//	@ID			get-string-by-int
//	@Accept			json
//	@Produce		json
//	@Param			some_id	path		int				true	"Some ID"
//	@Param			some_id	body	web.Pet			true	"Some ID"
//	@Success		200		{string}	string			"ok"
//	@Failure		400		{object}	web.APIError	"We need ID!!"
//	@Failure		404		{object}	web.APIError	"Can not find ID"
//	@Router			/testapi/get-string-by-int/{some_id} [get]
func GetStringByInt(w http.ResponseWriter, r *http.Request) {
}

@sdghchj
Copy link
Member Author

sdghchj commented Nov 25, 2022

@morremeyer
So, whether a space or a tab is not the key point.
If you have a solution to make all the versions compatible, PR is welcome.

@Zxilly
Copy link
Contributor

Zxilly commented Nov 25, 2022

Maybe add a setting file and allow user set format style?
Or add an additional option flag.

@morremeyer
Copy link
Contributor

@sdghchj Can I assume that you're referencing go 1.16 and go 1.19 respectively? While I wouldn't want to break compatibility with go 1.17 (even though it is not supported anymore) since 1.18 is only out for ~6 months, breaking compatiblity with 1.6 or 1.9 should not be an issue, should it?

@sdghchj
Copy link
Member Author

sdghchj commented Nov 29, 2022

Sorry, I have not got your point. I just have no better idea to fix it regarding all the preferences.
So if you have a good idea, PR is welcome.

@morremeyer
Copy link
Contributor

@sdghchj You mentioned go 1.6 and go 1.9 in your previous comment. Those versions of go are end of life for a while now, I do not see any problem in breaking compatibility with them.

I have researched this topic now and I think that swag is doing the right think here by using new features that are available with go 1.19.

This issue needs to be fixed in gofumpt which is breaking the correct formatting of swag fmt.

I opened mvdan/gofumpt#254 to track that. Thanks for your input and help here!

@morremeyer
Copy link
Contributor

I have continued to analyze this issue. The problem is neither with swag fmt nor with gofumpt or gofmt.

It is simply that with the new documentation behaviour in 1.19, you cannot have indented documentation without some non-indented documentation.

I had

//	@Summary		Create category
//	@Description	Creates a new category
//	@Tags			Categories
//	@Produce		json
//	@Success		201	{object}	CategoryResponse
//	@Failure		400	{object}	httperrors.HTTPError
//	@Failure		404
//	@Failure		500			{object}	httperrors.HTTPError
//	@Param			category	body		models.CategoryCreate	true	"Category"
//	@Router			/v1/categories [post]
func (co Controller) CreateCategory(c *gin.Context) {

which is not correctly formatted. It therefore gets re-formatted by gofmt.

With

// CreateCategory creates a new category
//
//	@Summary		Create category
//	@Description	Creates a new category
//	@Tags			Categories
//	@Produce		json
//	@Success		201	{object}	CategoryResponse
//	@Failure		400	{object}	httperrors.HTTPError
//	@Failure		404
//	@Failure		500			{object}	httperrors.HTTPError
//	@Param			category	body		models.CategoryCreate	true	"Category"
//	@Router			/v1/categories [post]
func (co Controller) CreateCategory(c *gin.Context) {

this does not happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants