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

pattern aliases #675

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

HiveTraum
Copy link

@HiveTraum HiveTraum commented May 15, 2022

✋Reopened #598 because i noticed that the project came to life

This feature introduces ability to register aliases for some often used regular expressions.

For example path as this:
/{category:[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}}/{product:[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}}
can be transformed into this
/{category:uuid}/{product:uuid}

You need just use register it

router := NewRouter()
router.RegisterPattern("uuid", "[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}")
router.Path("/{category:uuid}")
router.Path("/{product:uuid}")
router.Path("/{order:uuid}")
router.Path("/{user:uuid}")
  1. New method RegisterPattern on Route struct
  2. New method RegisterPattern on Router struct

@@ -0,0 +1,25 @@
package mux_test
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move this test case in the file: example_route_test.go

Copy link
Author

Choose a reason for hiding this comment

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

modev example

@@ -124,6 +126,14 @@ func copyRouteRegexp(r *routeRegexp) *routeRegexp {
return &c
}

func (r *Router) RegisterPattern(alias string, pattern string) *Router {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add comments here?

Copy link
Author

Choose a reason for hiding this comment

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

Added comments

@@ -94,6 +94,8 @@ type routeConf struct {
buildScheme string

buildVarsFunc BuildVarsFunc

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, can we add comment here as well?

Copy link
Author

Choose a reason for hiding this comment

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

Added comments

host: "aaa.bbb.ccc",
path: "",
hostTemplate: `{v-1:version}.{v-2:version}.{v-3:version}`,
shouldMatch: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add negative test case where shouldMatch is false because of mismatch.

Copy link
Author

Choose a reason for hiding this comment

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

Added negative test case

mux_test.go Outdated
},
{
title: "Path route with regexp alias patterns passed through router",
route: NewRouter().RegisterPattern("digits", "[0-9]+").Path("/{id:digits}"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Both the test cases look similar to me.

  1. First one:
{
	title:        "Path route with regexp alias patterns",
	route:        new(Route).RegisterPattern("digits", "[0-9]+").Path("/{id:digits}"),
	request:      newRequest("GET", "http://localhost/1"),
	vars:         map[string]string{"id": "1"},
	host:         "",
	path:         "/1",
	pathTemplate: `/{id:digits}`,
	shouldMatch:  true,
}
  1. Second:
{
	title:        "Path route with regexp alias patterns passed through router",
	route:        NewRouter().RegisterPattern("digits", "[0-9]+").Path("/{id:digits}"),
	request:      newRequest("GET", "http://localhost/1"),
	vars:         map[string]string{"id": "1"},
	host:         "",
	path:         "/1",
	pathTemplate: `/{id:digits}`,
	shouldMatch:  true,
}

We should add at least one negative test case. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, seems like this tests was very similar, deleted second test and added negative test case

@amustaque97
Copy link
Contributor

Thank you @HiveTraum for bringing this up again. I have added a couple of comments. I need some more time to review it completely. In the meantime, I Would love to hear from you.

@HiveTraum
Copy link
Author

@amustaque97 Done 👍

Copy link
Contributor

@amustaque97 amustaque97 left a comment

Choose a reason for hiding this comment

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

Thank you for working on comments. Changes looks good to me.

@amustaque97
Copy link
Contributor

@elithrar , I reviewed the PR and changes looks good to me and ready to ship 🚀

I would request you to please take a final look at this and if all good. Can we merge it?

@HiveTraum
Copy link
Author

HiveTraum commented Dec 5, 2022

Yes, it's ready to merge

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

Successfully merging this pull request may close these issues.

2 participants