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.

Sign up for GitHub

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

Jump to bottom

Refactor RoutePatternFactory to trim regex dependency where possible #46793

Merged
merged 2 commits into from Feb 22, 2023
Merged

Refactor RoutePatternFactory to trim regex dependency where possible #46793

merged 2 commits into from Feb 22, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Feb 22, 2023 •
edited

Fixes #46142

Creating regex constraint from a string is moved out of RoutePatternFactory.Parse(string) path, which all the MapXXX methods use.

I looked at this, and it was a pretty simple refactor. I'd like to see the improvement in preview 2, so I just went ahead and did it.

@ghost ghost added the area-runtime label Feb 22, 2023
mitchdenny
mitchdenny approved these changes Feb 22, 2023
View reviewed changes
Copy link
Member

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM.

@JamesNK JamesNK enabled auto-merge (squash) February 22, 2023 08:25
@JamesNK JamesNK merged commit f3836ae into main Feb 22, 2023
@JamesNK JamesNK deleted the jamesnk/trim-regex-constraint branch February 22, 2023 09:03
@ghost ghost added this to the 8.0-preview2 milestone Feb 22, 2023
captainsafia
captainsafia reviewed Feb 22, 2023
View reviewed changes
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Post-merge LGTM! This actually ended up being a much smaller delta than what I was expecting given that we were able to rip out just the parameter policy construction.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@captainsafia captainsafia captainsafia left review comments

@mitchdenny mitchdenny mitchdenny approved these changes

@javiercn javiercn Awaiting requested review from javiercn javiercn is a code owner

@eerhardt eerhardt Awaiting requested review from eerhardt

@halter73 halter73 Awaiting requested review from halter73

@brunolins16 brunolins16 Awaiting requested review from brunolins16

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions area-web-frameworks
Projects
None yet
Milestone
8.0-preview2
Development

Successfully merging this pull request may close these issues.

Resolve routing RegEx constraint dependency size issue
4 participants
@JamesNK @mitchdenny @captainsafia @amcasey