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

[BREAKING] Only match non-empty string #58

Merged
merged 2 commits into from Feb 24, 2022
Merged

[BREAKING] Only match non-empty string #58

merged 2 commits into from Feb 24, 2022

Conversation

ahopkins
Copy link
Member

Stemming from the conversation here: sanic-org/sanic-guide#92

This is a BREAKING change.

This this change, these will NOT match on an empty string.

router.add("/<foo>", ...)
router.add("/<foo:str>", ...)

This also adds <foo:strorempty> to allow for matching empty str patterns.

@Tronic
Copy link
Member

Tronic commented Jan 27, 2022

I suspect that this might be a breaking change for many who actually depend on str matching empty strings but I am still in favour of breaking it and having people migrate to strorempty or separate handlers/routes for such use cases.

Is this strorempty only supposed to be usable as the final part of an URL where the route has no trailing slash? Because you cannot have // in a path anyway, so I don't see where else this could be used.

EDIT: Turns out that HTTP actually allows consecutive slashes in path part of URLs, although I do expect those to break in many places. Nginx handles them as a single slash for file paths but logs and proxies keeping the multiple slashes.

Copy link
Member

@Tronic Tronic left a comment

Choose a reason for hiding this comment

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

LGTM but suggesting further consideration and documentation prior to merge (but with inclusion in 22.3).

Copy link
Member

@ashleysommer ashleysommer left a comment

Choose a reason for hiding this comment

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

Good change

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

Successfully merging this pull request may close these issues.

None yet

3 participants