-
-
Notifications
You must be signed in to change notification settings - Fork 7
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
initial work on router compiler #6
Conversation
1dece2b
to
5cd886b
Compare
5cd886b
to
15e7804
Compare
It might be nice to be able to pass in parameters to the URL matching engine via the |
An easy way to determine whether it's a regex path could be to check if it starts with |
I think it's better to be explicit. In general, we probably need some kind of Route( ... , url_resolver: Callable | None = DjangoResolver ) from dataclasses import dataclass
from typing import Callable, Iterable
@dataclass
class UrlResolver:
is_match: Callable[[str, Iterable[str]], bool] |
5d7170c
to
21543f0
Compare
@Archmonger, I've generalized the interface and edited the description. In short, you are now allowed to create compilers that operate on from reactpy_router import router as _router, Route as _Route
@dataclass
class Route(_Route):
regex: bool
# need a custom constructor due to the `*args` allowed for routes in the base class
def __init__(self, *args, regex: None | bool = None, **kwargs):
self.regex = regex
super().__init__(*args, **kwargs)
def router(*routes: Route):
return _route(*routes, compiler=compile_django_route)
def compile_django_route(route: Route) -> DjangoRoutePattern:
...
class DjangoRoutePattern:
... See above for expected interface for |
21543f0
to
a33b8f9
Compare
This allows compilers to work in a wider variety of ways.
a33b8f9
to
54d6776
Compare
I'll take a peek at it soon. Luckily today is the first time in a while I'm actually going to have some free time for dev. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Needs docs on how to use this in the readme.
- I'm also now noticing there's still literally no info on how to use idom router in the readme.
- Needs more comments across the new code.
- There's a bit of over-privatization of variables going on. I think it's best to only underscore things that, if the user was to touch it, would fundamentally break things.
I'm practically done with everything on my Conreq PR besides integrating URL routing. So this PR has officially become a hard block for me. |
Notes from review:
|
5a92ffb
to
f23c06a
Compare
|
EDIT: The suggestion below wouldn't match What if rather than us compiling the paths within the route handler, we provide a function that compiles the provided path when provided? class Path:
def __init__(self, path: str):
self.path = path
self.something_to_be_cached = ...
...
def __repr__(self):
return self.path
def resolve(self, match: str) -> bool:
...
Path("^this_is_my_path/") This would require all router paths to be This would allow regex caching to occur each |
Will review tomorrow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than a missed print statement, looks good to me.
I believe this PR also closes #13
closes: #5
closes: #13
Depends on: reactive-python/reactpy#823
The interface for the compiler is a simple callable of the form:
Where
RoutePattern
is any object of the form:The value returned by
match
isNone
to indicate a non-match or a dictionary of parameters that were discovered in the path string.