feat(routing): add regex converter support in routing - #2715
feat(routing): add regex converter support in routing#2715infraredCoding wants to merge 4 commits into
Conversation
Add new builtin-converter RegexConverter to allow regex pattern matching when adding routes. Closes falconry#857
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2715 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 7990 8004 +14
Branches 1106 1107 +1
=========================================
+ Hits 7990 8004 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
vytas7
left a comment
vytas7
left a comment
There was a problem hiding this comment.
Hey @infraredCoding , and thanks for this PR.
It is a solid start, however, we need to address a couple minor issues before merging:
- A towncrier newsfragment is needed for this new feature. In this case the file should be named
857.newandimproved.rst. - It seems there are a couple of stray lines without code coverage. We need to add missing tests as Falcon requires 100% line and branch coverage.
- Not critical, but it would be nice to add the new class to the table summarizing the available converters (in the docs).
Add new newsfragment for changelog regarding RegexConverter. Add RegexConverter usage in docs Closes falconry#857
Add new tests for covering bad regex pattern and exception handling Closes falconry#857
|
@vytas7 Thanks a lot for the feedback. I have added the newsfragment and the new test cases. Also, updated the routing docs |
| __slots__ = ('_pattern', '_compiled') | ||
|
|
||
| def __init__(self, pattern: str) -> None: | ||
| import re |
There was a problem hiding this comment.
Is there any specific reason to import re here inline?
Falcon's default router already uses re for sub-field matching, so this doesn't really win us anything.
There was a problem hiding this comment.
No there was no particular reason I did that. I suppose it was simply just habit.
| ) from ex | ||
|
|
||
| def convert(self, value: str) -> str | None: | ||
| if self._compiled.fullmatch(value): |
There was a problem hiding this comment.
A more conceptual question here is whether we should support anything related groups, i.e., match the first group if it exists, or somehow support passing the information from the named groups, if any?
@CaselIT what do you think?
2 participants
Add new builtin-converter RegexConverter to allow regex pattern matching when adding routes.
Closes #857
Summary of Changes
Add Regular Expression route conversion support
Related Issues
Closes #857
Pull Request Checklist
tox.docs/.docs/.versionadded,versionchanged, ordeprecateddirectives.docs/_newsfragments/, with the file name format{issue_number}.{fragment_type}.rst. (Runtox -e docs, and inspectdocs/_build/html/changes/in the browser to ensure it renders correctly.)This is my first PR in this repo and I might not be entirely sure how to change the documentations.
PR template inspired by the attrs project.