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

Bugfix for the FullPath feature #1919

Merged
merged 4 commits into from
Jun 28, 2019
Merged

Bugfix for the FullPath feature #1919

merged 4 commits into from
Jun 28, 2019

Conversation

bbiao
Copy link
Contributor

@bbiao bbiao commented May 30, 2019

  • worked with more complex situations
  • the original pr not work when add a short route with the same prefix
    to some already added routes
  • With pull requests:
    • Open your pull request against master
    • Your pull request should have no more than two commits, if not you should squash them.
    • It should pass all tests in the available continuous integrations systems such as TravisCI.
    • You should add/modify tests to cover your proposed code changes.
    • If your pull request contains a new feature, please document it on the README.

 * worked with more complex situations
 * the original pr not work when and a short route with the same prefix
 to some already added routes
@bbiao
Copy link
Contributor Author

bbiao commented May 30, 2019

@zaynetro Here is some fix for your recently commit of the #1826

@codecov
Copy link

codecov bot commented May 30, 2019

Codecov Report

Merging #1919 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1919      +/-   ##
==========================================
+ Coverage   98.68%   98.69%   +<.01%     
==========================================
  Files          40       40              
  Lines        2212     2218       +6     
==========================================
+ Hits         2183     2189       +6     
  Misses         16       16              
  Partials       13       13
Impacted Files Coverage Δ
tree.go 100% <100%> (ø) ⬆️

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 fc920dc...804e5f9. Read the comment docs.

@appleboy appleboy added the bug label May 30, 2019
@appleboy appleboy added this to the 1.5 milestone May 30, 2019
@zaynetro
Copy link
Contributor

Good catch!

@thinkerou
Copy link
Member

the pr branch benchmark data:

➜  go-http-routing-benchmark git:(master) ✗ go test -bench=Gin
#GithubAPI Routes: 203
   Gin: 58560 Bytes

#GPlusAPI Routes: 13
   Gin: 4368 Bytes

#ParseAPI Routes: 26
   Gin: 7744 Bytes

#Static Routes: 157
   Gin: 34064 Bytes

goos: darwin
goarch: amd64
pkg: github.com/julienschmidt/go-http-routing-benchmark
BenchmarkGin_Param        	20000000	        95.1 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param5       	10000000	       179 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param20      	 3000000	       533 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParamWrite   	10000000	       175 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubStatic 	10000000	       126 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	10000000	       231 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubAll    	   30000	     48530 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusStatic  	20000000	        98.9 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusParam   	10000000	       152 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlus2Params 	10000000	       211 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusAll     	 1000000	      2131 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseStatic  	20000000	        99.2 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseParam   	20000000	       102 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Parse2Params 	10000000	       129 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseAll     	  500000	      3344 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_StaticAll    	   50000	     25523 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/julienschmidt/go-http-routing-benchmark	32.030s

and master branch benchmark:

➜  go-http-routing-benchmark git:(master) ✗ go test -bench=Gin
#GithubAPI Routes: 203
   Gin: 58480 Bytes

#GPlusAPI Routes: 13
   Gin: 4368 Bytes

#ParseAPI Routes: 26
   Gin: 7744 Bytes

#Static Routes: 157
   Gin: 34064 Bytes

goos: darwin
goarch: amd64
pkg: github.com/julienschmidt/go-http-routing-benchmark
BenchmarkGin_Param        	20000000	        91.2 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param5       	10000000	       185 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Param20      	 3000000	       528 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParamWrite   	10000000	       177 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubStatic 	10000000	       124 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubParam  	10000000	       223 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GithubAll    	   30000	     46198 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusStatic  	20000000	        88.5 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusParam   	10000000	       133 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlus2Params 	10000000	       186 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_GPlusAll     	 1000000	      1958 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseStatic  	20000000	        94.2 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseParam   	20000000	       111 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_Parse2Params 	10000000	       135 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_ParseAll     	  500000	      3445 ns/op	       0 B/op	       0 allocs/op
BenchmarkGin_StaticAll    	   50000	     26084 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	github.com/julienschmidt/go-http-routing-benchmark	31.207s

@bbiao
Copy link
Contributor Author

bbiao commented Jun 6, 2019

Will this pr be merged ? @thinkerou

@thinkerou thinkerou requested a review from appleboy June 6, 2019 03:58
@thinkerou
Copy link
Member

@bbiao also need @appleboy review. thanks!

@thinkerou
Copy link
Member

@appleboy please review the pull request, thanks!

@appleboy appleboy merged commit f65018d into gin-gonic:master Jun 28, 2019
ThomasObenaus pushed a commit to ThomasObenaus/gin that referenced this pull request Feb 19, 2020
* worked with more complex situations
 * the original pr not work when and a short route with the same prefix
 to some already added routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants