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

Add literal colon support (#1432) #2857

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

wssccc
Copy link
Contributor

@wssccc wssccc commented Sep 6, 2021

Implement literal colon support using the approach described in #1432

@wssccc wssccc changed the title Add escaped colon support (#1432) Add literal colon support (#1432) Sep 6, 2021
@wssccc wssccc force-pushed the master branch 2 times, most recently from c1585c2 to 2e6a3ff Compare September 6, 2021 09:02
@codecov
Copy link

codecov bot commented Sep 6, 2021

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.37%. Comparing base (3dc1cd6) to head (c46a545).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2857      +/-   ##
==========================================
+ Coverage   99.21%   99.37%   +0.15%     
==========================================
  Files          42       43       +1     
  Lines        3182     2711     -471     
==========================================
- Hits         3157     2694     -463     
+ Misses         17        9       -8     
  Partials        8        8              
Flag Coverage Δ
?
-race ∅ <ø> (?)
-tags go_json 99.36% <100.00%> (?)
-tags nomsgpack 99.36% <100.00%> (?)
go-1.18 99.29% <100.00%> (+0.17%) ⬆️
go-1.19 99.37% <100.00%> (+0.15%) ⬆️
go-1.20 99.36% <100.00%> (+0.14%) ⬆️
go-1.21 ?
macos-latest ?
ubuntu-latest 99.37% <100.00%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wssccc wssccc force-pushed the master branch 4 times, most recently from 464c884 to ed0a30c Compare September 6, 2021 09:56
Copy link
Contributor

@daheige daheige left a comment

Choose a reason for hiding this comment

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

Still need to optimize and adjust, why define some routing rules like \?

gin.go Outdated Show resolved Hide resolved
@wssccc
Copy link
Contributor Author

wssccc commented Sep 6, 2021

Still need to optimize and adjust, why define some routing rules like ?

The feature included in this PR is not about defining rule like \, but using \ to escape a colon

Please refer to issue #1432 for more details

@wssccc wssccc force-pushed the master branch 2 times, most recently from 02a7101 to 6f0bdd0 Compare September 7, 2021 05:33
gin.go Outdated Show resolved Hide resolved
tree.go Outdated Show resolved Hide resolved
daheige
daheige previously approved these changes Apr 22, 2022
@wssccc
Copy link
Contributor Author

wssccc commented Apr 22, 2022

@cikay @daheige conflicts and issues mentioned above have been resolved

@mytram
Copy link

mytram commented Jul 13, 2022

Need this for my project which follows google's custom method conventions, i.e. :verb I am wondering if @cikay @daheige can review it and give it a tick. That will be awesome. Many thanks

@mytram
Copy link

mytram commented Jul 13, 2022

@daheige Thanks a lot 👍 🙏 !

@mytram
Copy link

mytram commented Jul 13, 2022

Hi @thinkerou wondering if you can have a look and approve the PR for the next release. Many thanks 🙏

@slnc
Copy link

slnc commented Nov 28, 2023

@cikay @daheige is anything blocking this PR? I'm asking in case the community can help. This PR would fix an issue my team is having.

Thank you

@appleboy appleboy added this to the v1.10 milestone Mar 11, 2024
@appleboy
Copy link
Member

I will take it asap.

@appleboy appleboy modified the milestones: v1.10, v1.11 Mar 21, 2024
f := func(u string) {
router.GET(u, func(c *Context) { c.String(http.StatusOK, u) })
}
f("/r/r\\:r")
Copy link

Choose a reason for hiding this comment

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

Should probably add a test case for \:r as well, which should not result in expected behaviour.

f("/r/r/\:r")

for start, c := range []byte(path) {
if escapeColon {
if c == ':' {
escapeColon = false
Copy link

Choose a reason for hiding this comment

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

What about a case of \\foo:? escapeColon would stay true after escape character until colon is reached. It's probably not expected behaviour. It should try to escape next character by the rules. This case is also not covered with test.

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

Successfully merging this pull request may close these issues.

None yet

7 participants