Allow different param names in different methods with same path scheme#2209
Allow different param names in different methods with same path scheme#2209aldas merged 5 commits intolabstack:masterfrom
Conversation
|
@aldas could you run tests again? Added small fix |
|
I forgot about middleware tests. Fixed |
Signed-off-by: ortyomka <iurin.art@gmail.com>
ec17a4a to
3e45d2a
Compare
|
Found a quicker and easier way for the new structure. In old - original original vs current cc @aldas, ready for review |
aldas
left a comment
There was a problem hiding this comment.
I do not understand why this new struct methodContext is necessary in this PR context? I do not see it adding any functional or performance value. Changing things from HandlerFunc to pointer seems unnecessary as function types are nillable.
Agree, I will add functionality |
Signed-off-by: ortyomka <iurin.art@gmail.com>
|
Added functionality, to store different paths and parameters in different methods Done @aldas |
|
@aldas , if you have time, please review PR |
router.go
Outdated
| routes map[string]*Route | ||
| echo *Echo | ||
| } | ||
| methodContext struct { |
There was a problem hiding this comment.
I do not think that context describes this struct correctly. In world of http servers and Echo context is something more temporary. I have used routeMethod in v5 for similar struct but in v5 methodHandler struct is also renamed to routeMethods.
There was a problem hiding this comment.
Renamed to routeMethod
Add paramsCount in each node for perfomance Signed-off-by: ortyomka <iurin.art@gmail.com>
|
@aldas I have fixed comments, could you take a look? |
Codecov Report
@@ Coverage Diff @@
## master #2209 +/- ##
==========================================
+ Coverage 92.26% 92.29% +0.02%
==========================================
Files 37 37
Lines 3090 3102 +12
==========================================
+ Hits 2851 2863 +12
Misses 150 150
Partials 89 89
Continue to review full report at Codecov.
|
router.go
Outdated
| // so we can send http.StatusMethodNotAllowed (405) instead of http.StatusNotFound (404) | ||
| currentNode = previousBestMatchNode | ||
|
|
||
| ctx.path = path |
There was a problem hiding this comment.
See comment below. This is incorrect as path must be path of the route not path of the request.
Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
|
I would like to finish this PR by Tuesday(12.07) inclusive. If your schedule allows it, I'm willing to fix comments ASAP. CC @aldas |
aldas
left a comment
There was a problem hiding this comment.
LGTM
I think this PR is good enough for current state. Params for 405 etc are things we/I can address later when there is a decision what the behavior should be. These questions are not necessary to be solved with this change.
Thank you for PR.
Relates to issues #1726 and #2201
Parameters and paths are now separated by methods within the same node.
Add new method Context, which store path, parameters, and handler.
Add test from #1726.
CC @aldas