django-rest-framework
django-rest-framework copied to clipboard
encode
Add SimplePathRouter
Description
Add SimplePathRouter to handle Django 2.x path converters (refs #6148).
It may be improved a bit if support for Django 1.x gets dropped (ie: some bad if will be removed).
I'm thinking this should be pushed to the 3.11 release, as that is when we'll likely drop Django 1.11
@rpkilby I have updated with CRUD tests and removed Django 1.11 compact. Tell me if everything is fine for you.
can you skip tests for Django 1.11 for this PR? dj1.11 seems to be supported for some more time.
I have also added an assertion in SimplePathRouter.get_urls to raise an assertion error if path is not defined.
Ok... seems that pytest.mark.skipif has not worked as expected. I will recheck it as soon as I can.
We've not dropped 1.11 on this pass, since its LTS.
Wondering if we might just outright switch the behavior on the next release - I'd rather we didn't have two different have two different class implementations here.
@tomchristie you are right that a single-class implementation would be better.
I think that it can be done since the difference between those two are just two methods (get_urls and get_get_lookup_*). Those methods could be guided with some initialization param in class constructor.
I will try to refactor the code to have a single class.
When we drop 1.11 I'd actually probably be okay with us just completely switching to .path, so long as there's some kind of way for users to stick with the older behavior.
There's not really any great options for us here.
@tomchristie could you please check if the latest implementation is fine for you?
I've tested this patch on 3.12.2 (after a cherry-pick) in my project and it certainly does what it says on the tin.
Tox on a sedded version of the tests (s, re_path(r'^, path(',g) has no complaints (if we disregard a failure in an AuthToken test on django_master), so I think I'm gonna vendor this while waiting for 3.13. W00T!
Is there anything preventing a merge of this?
time to review and merge from the core maintainers side I guess.
REST framework these days is in the somewhat odd position of both being the primary driver of our sponsorships, while also being feature complete and generally not in need of any significant development beyond keeping pace with upcoming Django releases.
Work here probably needs to be focused on good communication aimed at minimising the workload needed to keep the project well maintained.
from https://www.encode.io/reports/december-2020
@auvipy Tests were failing cause there was a problem with travis (missing six package), I have rebased and pushed my changes.
This would be really nice in order to have custom Django path converters be used in DRF. Is there any other workarounds without this PR?
I have done a small refactor removing compatibility for missing path (in test) and replaced default path converter from str to path (which has the same regex which is used by default in routers).
@auvipy Any issues merging this, running into a similar problem that can be simplified using path converters.
@auvipy Any issues merging this, running into a similar problem that can be simplified using path converters. I am not the right person to merge this. @tomchristie @rpkilby they are the maintainers.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.
![stale[bot] avatar](https://avatars.githubusercontent.com/in/1724?v=4 "Published by a pub.dev verified publisher"){kind=small}
Since it has been more than 2 years since the first draft of this PR, I am start thinking: is there a more elegant way to handle path?
I was wondering if using the include could make the router a bit more usable and reduce string manipulations which could cause hard-to-discover bugs for end users.
This PR is a starting step, but I which the feature could be improved.
Also waiting for this feature to be merged.
Also, as we are on Django 2.x+, should be the path be the default for SimpleRouter or DefaultRouter?
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.