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

WIP: experiment to convert macaron to gin #7177

Closed
wants to merge 12 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 11, 2019

This PR is an experiment to convert macaron to gin.
It moved all static files from macaron's middleware to gin's.
It made macaron as a middleware of gin, so that we can keep most of routes and move step by step.

@codecov-io
Copy link

codecov-io commented Jun 11, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@46d3d10). Click here to learn what that means.
The diff coverage is 45.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7177   +/-   ##
=========================================
  Coverage          ?   41.73%           
=========================================
  Files             ?      450           
  Lines             ?    61143           
  Branches          ?        0           
=========================================
  Hits              ?    25519           
  Misses            ?    32307           
  Partials          ?     3317
Impacted Files Coverage Δ
routers/routes/routes.go 83.79% <100%> (ø)
routers/routes/gin_bridge.go 43.47% <43.47%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46d3d10...96e13b4. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 11, 2019
@techknowlogick techknowlogick added this to the 1.10.0 milestone Jun 12, 2019

// for health check
g.HEAD("/", func(c *gin.Context) {
c.String(200, "")
Copy link
Member

Choose a reason for hiding this comment

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

c.AbortWithStatus(http.StatusOK)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@lunny lunny force-pushed the lunny/macaron_gin_bridge branch 3 times, most recently from e586b73 to 17f70b2 Compare June 16, 2019 14:44
func SignedUserName(ctx *gin.Context) string {
if v, ok := ctx.Get("SignedUserName"); !ok {
return ""
} else {
Copy link
Member Author

Choose a reason for hiding this comment

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

@kolaente golangci-lint failed because

routers/routes/gin.go:35:9: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (golint)
--
583 | } else {
584 | ^
585 | Makefile:471: recipe for target 'golangci-lint' failed

But I don't think there is another better style. :(

Copy link
Member

Choose a reason for hiding this comment

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

What about something like

var uname string
if v, ok := ctx.Get("SignedUserName"); ok {
  uname = v.(string)
}
return uname

Copy link
Member

Choose a reason for hiding this comment

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

@lunny Exactly what @jolheiser meant is needed here.

@lunny
Copy link
Member Author

lunny commented Jun 17, 2019

@zeripath I think your suggest is not right. v is a local variable only available in if or else.

@zeripath
Copy link
Contributor

Oh I see. Switch em round?

@zeripath
Copy link
Contributor

zeripath commented Jun 17, 2019

So

func SignedUserName(ctx *gin.Context) string {
	if v, ok := ctx.Get("SignedUserName"); ok {
		return v.(string)
	}
	return ""
}

@lafriks lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Jun 17, 2019
@lunny lunny force-pushed the lunny/macaron_gin_bridge branch from cfebfaa to 57d0301 Compare July 9, 2019 04:42
@vtolstov
Copy link

vtolstov commented Jul 9, 2019

Can you provide some info why gin and not chi ?

@lunny
Copy link
Member Author

lunny commented Jul 9, 2019

@vtolstov No reason from me. Just an experiment. I post a vote issue, see #7403

@vtolstov
Copy link

vtolstov commented Jul 9, 2019

Thank you

@techknowlogick techknowlogick removed this from the 1.10.0 milestone Sep 3, 2019
@techknowlogick techknowlogick added this to the 1.11.0 milestone Sep 3, 2019
@stale
Copy link

stale bot commented Nov 2, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

@stale stale bot added the issue/stale label Nov 2, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Nov 2, 2019
@stale stale bot removed the issue/stale label Nov 2, 2019
@techknowlogick techknowlogick modified the milestones: 1.11.0, 1.x.x Dec 12, 2019
@lunny
Copy link
Member Author

lunny commented May 2, 2020

related with #7403

@lunny
Copy link
Member Author

lunny commented Nov 12, 2020

Closed by #7420

@lunny lunny closed this Nov 12, 2020
@lunny lunny deleted the lunny/macaron_gin_bridge branch November 18, 2020 04:28
@lunny lunny removed this from the 1.x.x milestone Nov 19, 2020
@go-gitea go-gitea locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants