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

Replaced double quotes with backticks in all route parameter strings #2591

Merged
merged 5 commits into from
Sep 4, 2023

Conversation

11-aryan
Copy link
Contributor

@11-aryan 11-aryan commented Aug 22, 2023

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Explain the details for making this change. What existing problem does the pull request solve?

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • For new functionalities I follow the inspiration of the express js framework and built them similar in usage
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - /docs/ directory for https://docs.gofiber.io/
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
  • I tried to make my code as fast as possible with as few allocations as possible
  • For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

@welcome
Copy link

welcome bot commented Aug 22, 2023

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@Skyenought
Copy link
Member

@11-aryan Why not use double quotes? Please give a reason

@11-aryan
Copy link
Contributor Author

11-aryan commented Aug 25, 2023

@Skyenought As pjebs had mentioned in the issue description, in go's regex documentation, they always use backticks to make sure it is unambiguous and the escape character doesn't interfere with regex patterns in an unexpected way.

@ReneWerner87 ReneWerner87 linked an issue Aug 25, 2023 that may be closed by this pull request
3 tasks
@ReneWerner87
Copy link
Member

ReneWerner87 commented Aug 25, 2023

understandable, but please only where escaped, the normal case should already be the double quote

from issue report:
image

currently im PR
image

can we also adjust the test which is in the code with the backticks to make sure that works what we mention in the docs

pattern: "/v1/some/resource/name\\:customVerb",
testCases: []routeTestCase{
{url: "/v1/some/resource/name:customVerb", params: nil, match: true},
{url: "/v1/some/resource/name:test", params: nil, match: false},
},
},
{
pattern: "/v1/some/resource/:name\\:customVerb",
testCases: []routeTestCase{
{url: "/v1/some/resource/test:customVerb", params: []string{"test"}, match: true},
{url: "/v1/some/resource/test:test", params: nil, match: false},
},
},
{
pattern: "/v1/some/resource/name\\\\:customVerb?\\?/:param/*",
testCases: []routeTestCase{
{url: "/v1/some/resource/name:customVerb??/test/optionalWildCard/character", params: []string{"test", "optionalWildCard/character"}, match: true},
{url: "/v1/some/resource/name:customVerb??/test", params: []string{"test", ""}, match: true},
},

there are also other cases
https:/search?q=repo%3Agofiber%2Ffiber+%5C%5C%5C%5C+path%3Apath_testcases_test.go&type=code

@11-aryan
Copy link
Contributor Author

Yeah sure, I'll change it

@ReneWerner87
Copy link
Member

@11-aryan any progress ?

@11-aryan
Copy link
Contributor Author

11-aryan commented Sep 3, 2023

@ReneWerner87 I've made the required changes.

@ReneWerner87
Copy link
Member

Ok. Thanks

@ReneWerner87 ReneWerner87 merged commit 328411a into gofiber:master Sep 4, 2023
20 checks passed
@welcome
Copy link

welcome bot commented Sep 4, 2023

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

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.

🐛 [Bug]: Documentation for route escape
4 participants