-
Notifications
You must be signed in to change notification settings - Fork 84
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
Simplified routing syntax #27
Conversation
|
||
urlpatterns = [ | ||
path('/users/', views.user_list), | ||
path('/users/<id>/', views.user_detail), |
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.
The previous example left off the leading slash, but this one has it. I suspect that we want the former, but which is correct.
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.
Yes, I just copied & pasted that from the original proposal from Tom Christie, but other parts of the DEP are clear that it is about path components and not URLs, so this will have to change.
The route argument to the ``path`` function signifies a component of the full URL, and as such should (normally) not start with a ``/``. Thanks Ryan Hiebert.
|
||
* Enforce that new style ``path()`` arguments must not start with a leading | ||
``'^'``. | ||
* Enfore that old style ``url()`` arguments must start with a leading ``'^'``. |
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.
Typo: "Enforce".
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.
Thanks! I fixed it and pushed a new commit.
I love this proposal, for the record. It obviates a tiny library I made for myself, in a great way. |
Thanks Kit La Touche!
Okay, I think this is ready for a formal DEP #, right? |
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.
I flagged a few minor spelling errors, but otherwise looks good! Django's URL routing is a point of personal frustration (I came from Flask to Django), so I'd be very pleased if this got fixed. 😄
|
||
There are two aspects to this that we'd like to improve on: | ||
|
||
* The Regex based URL syntax is unneccessarily verbose and complex for the vast |
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.
/s/unneccessarily/unnecessarily
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.
Thanks for the spelling reports! I've just fixed them.
|
||
We might also consider including `a regex converter <http://stackoverflow.com/questions/5870188/does-flask-support-regular-expressions-in-its-url-routing>`_. | ||
|
||
Furthermore, an interface for implementating custom converters should exist. We |
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.
/s/implementating/implementing
* The items in the ``converters`` argument would each be instances of | ||
``BaseConverter`` | ||
|
||
(An alternate might be to add separate ``converter_args`` and |
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.
/s/alternate/alternative
Preventing unintended errors | ||
---------------------------- | ||
|
||
*The following behaviour is not neccessary, and we might not choose to add |
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.
/s/neccessary/necessary
Implementation tasks | ||
==================== | ||
|
||
The following independant tasks can be identified: |
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.
/s/independant/independent
@tomchristie: Now that I've fixed the spelling issues, I think it is ready for a formal DEP #. |
I've had a skim through and I feel this is ready to be merged into the drafts folder. @tomchristie - you should have the commit access to do that? |
@mjtamlyn - Nope, I don't have commit access. Yup, agree that it's ready for |
Assigned number 0201 (seemed unlikely it would get done by 1.11 feature freeze in January) and I couldn't work out how to fit 1.11 into the number scheme anyway. Merged into master in 5b306de |
Does that have any impact, or is it just symbolic? I'd like to aim to get it in for 1.11 and can make some time to help with that if needed. |
It's pretty much symbolic the number yeah. If you can get it all working and merged by the feature freeze then great, but it's still just a draft. We'll need a concrete plan and a technical board vote to get it to accepted, and then a mergable patch to get it to final. |
The current state of the pull request is pretty far along django/django#7482 The "Parametrisable converter syntax" is the one substantial technical aspect of this that's still TODO. (Plenty to do in the documentation and a couple of more minor dev aspects to address) Putting that aside I don't see any reason for us not to starting to addressing the potential blockers for this getting in, namely:
|
If it helps, I would not mind shelving the parametrisation, making it more likely that this can be completed before the deadline. (Also, I think it is a bit more controversial and a bit less specified than the rest of the proposal, to be honest). |
|
I'm concerned about the lack of technical details regarding reversing of these new URLs in the DEP. I see from the PR that some of this has been solved? The approaches should be mentioned here. I'd also like to see how the direction of any internal reworking here can be combined with Andrew's routing taking other attributes in channels and with django-hosts. I'm happy for the conclusion to be that this could be a future piece of work, but I'd like to see it discussed within the DEP. |
I agree with respect to docs and the flattened import paths. For deprecation, I think the The part regarding reversing has not been mentioned in the DEP indeed, but are a solvable problem. I'll create a PR updating DEP0201 to document the changes needed the reversing (API and internals). As for the Channels and Django-Hosts: Sounds interesting, but I think for now it needs to be "future work" as
Again, I'll see if I can reword this a bit more elegantly for in DEP0201. |
Docs patch, in progress: django/django#7542 |
I created a PR with some amendments: #34 |
Mostly a rewrite of https://gist.github.com/tomchristie/cb388f0f6a0dec931c611775f32c5f98 in rST.