Skip to content

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

Merged
ReneWerner87 merged 5 commits intogofiber:masterfrom
11-aryan:11-aryan
Sep 4, 2023
Merged

Replaced double quotes with backticks in all route parameter strings #2591
ReneWerner87 merged 5 commits intogofiber:masterfrom
11-aryan:11-aryan

Conversation

@11-aryan
Copy link
Copy Markdown
Contributor

@11-aryan 11-aryan commented Aug 22, 2023 •
edited
Loading

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
Copy Markdown

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

@gaby gaby requested review from ReneWerner87 and efectn August 24, 2023 13:31
@Skyenought
Copy link
Copy Markdown
Member

Skyenought commented Aug 24, 2023

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

@11-aryan
Copy link
Copy Markdown
Contributor Author

11-aryan commented Aug 25, 2023 •
edited
Loading

@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
🐛 [Bug]: Documentation for route escape #2534
Closed
3 tasks
@ReneWerner87
Copy link
Copy Markdown
Member

ReneWerner87 commented Aug 25, 2023 •
edited
Loading

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

fiber/path_testcases_test.go

Lines 88 to 106 in 8ec7cec

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://github.com/search?q=repo%3Agofiber%2Ffiber+%5C%5C%5C%5C+path%3Apath_testcases_test.go&type=code

@11-aryan
Copy link
Copy Markdown
Contributor Author

11-aryan commented Aug 26, 2023

Yeah sure, I'll change it

@ReneWerner87
Copy link
Copy Markdown
Member

ReneWerner87 commented Sep 1, 2023

@11-aryan any progress ?

@11-aryan
Copy link
Copy Markdown
Contributor Author

11-aryan commented Sep 3, 2023

@ReneWerner87 I've made the required changes.

@ReneWerner87
Copy link
Copy Markdown
Member

ReneWerner87 commented Sep 3, 2023

Ok. Thanks

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

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

Reviewers

@gaby gaby gaby approved these changes

@ReneWerner87 ReneWerner87 ReneWerner87 approved these changes

@efectn efectn Awaiting requested review from efectn

Assignees

No one assigned

Labels

📒 Documentation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: Documentation for route escape

4 participants

@11-aryan @Skyenought @ReneWerner87 @gaby