drf-nested-routers
drf-nested-routers copied to clipboard
alanjds
a pull request for review
Here I provide codes & tests I wrote one year ago for you to review.
You may want to specify what this pull request is about. I'm getting the impression that it's not really supposed to be a pull request (the code does not belong in drf-nested-routers), it's just something you like to get reviewed, am I right?
@yiakwy Sorry, but is kinda hard to review this much code (>1600 lines) without knowing what this does, nor how is it done. Can you please explain what this PR does, and how this is util to other users of drf-nested-routers?
@It is easy to know what it does!
First we compose a generic router to bind all methods to our methods in view.py or viewset.py. You check test.py or other files.
Secondly, instead using defaut router, we create a router dynamically bind methods and generate methods using template inside it.
Other codes is to concatenate method name with our resource name so that a url -> method automatically generated as what we want!
What's the purpose of this? The PR doesn't really have a descriptive title or any other message which tells us what issue this PR addresses.
It is about nested router. I want enhance our lib using codes I did a long time ago. ^_^. You can see complement.md
Your PR is only useful for your own, very specific use of this library, as far as I can tell from a quick glance. Thousands of lines of specific code. Not generally useful for just any user of drf-nested-routers. Basically you've written an entire app around what is essentially quite a small library, so I don't think this should be merged. Unless I'm missing the point entirely?
I think the large quantity of custom code is the toy environment (somewhat common in Django add-ons) that supports the test case. I think the substance of the PR is built on an argument like:
- In the examples for this project, all URLs follow a structure that's could be described by the (not strictly) regex
(resource-type/id/)*resource-type/(id/)? - If all REST URLs followed this structure, there would be no need to manually code routers. It should be possible to deduce the intended view from the last
resource-type, the view method from the presence/absence ofid(plus the HTTP verb) and all the constraints from the ancestor segments.
If I'm right, I sympathize with the logic of the proposal. We have a ModelSerializer and a ModelViewSet because the defaults are predictable and the "magic" is extremely DRY. This is a little like proposing a ModelRouter. I think the code actually deduces routes from Views (not models) so it's actually (and rightly) resource-centric rather than model-centric.
My concerns are mostly about "edge cases". I could come up with a dozen real-life scenarios that could break the default... but I'm not sure if any of them are actually RESTful. For example, this approach would have a problem if we returned a different representation of the same resource at /related-type-1/id/focal-type/ and /realated-type-2/id/focal-type/. But is that RESTful?
@claytondaley Thanks for the clarification and for digging into the code more than I did.
If this is the point, I see as valuable too. Would be nice to register only the "root" router.
The two strengths of the actual code are, in my opinion, simplicity and composition. Would like to see this new proposed API implemented by composing the actual simple parts together assuming a "default" structure. Maybe this way the "edge cases" became less critical as, when occur, one can disassembly the composition and do their stuff "by hand". Or send us a PR fixing the corner case 😬.