Hi Marten, I had a look at your proposal today. You've done good work there.
My comments revolve around making the API obvious, simplifying it until someone who discovers it understands immediately what the moving pieces are, what they do, and how they relate to one another. Here are my two main reactions. 1) The interplay between URL and Constraint is unclear. In particular I'm not sure which one is designed to be customized, perhaps by subclassing. To what extent are constraints expected to poke into the URL's internals? The documentation shows that URL has methods that accept Constraint in argument and Constraint has methods that accept URL in argument. Often such dependency loops indicate that responsibilities should be split differently between the two classes. I also share Carl's concerns with regards to URL being mutable. I don't have a ready-made solution to offer but two suggestions: a. Try to organize classes in a tree where parent classes use their children's API but children don't known anything about their parents. b. Try to clarify further the responsibilities of each class and make sure they don't know more than they need to achieve this purpose. For example, having Constraint.match() accept a string in argument -- the current URL path -- instead of an URL object would prevent Constraint from depending on URL's API. 2) The Resolver / View API can be simplified. Is there a fundamental reason to have two different classes here? If so, is the split in responsibilities optimal? As far as I can tell, there's some overlap between the two classes, especially in `resolve()`, but: - Resolver has additional code for resolving further - View has additional code for calling a view My gut feeling is that View should be subsumed into Resolver. Finally, if you haven't already, you should read about Simon Willison's theory of "Turtles all the way down": https://web.archive.org/web/20121013085920/http://simonwillison.net/2009/May/19/djng/ The general idea is that at any point in the Django stack (down to the view callable) you have stuff that sees an incoming request and will eventually return a response. I don't think it's directly relevant to your work but it can provide some guiding priciples. I hope this helps! -- Aymeric. > On 4 mai 2015, at 23:04, Carl Meyer <[email protected]> wrote: > > Hi Marten, > > On 05/04/2015 01:45 PM, Marten Kenbeek wrote: >> I'd like to discuss the API I'm proposing for the new url dispatcher I'm >> working on. I'll try to explain the API and some of the rationale behind it. > > Thanks for seeking comment early, and providing working PoC code, prose > description, and API example! > >> There is a working proof-of-concept >> at https://github.com/knbk/django/tree/url_dispatcher. Currently, all >> the names are chosen as not to conflict with the old implementation. A >> short description can be found >> at https://gist.github.com/knbk/96999abaab4ad4e5f8f9, which also >> contains a link to an example url configuration. >> >> My main goal was to create a simple, extensible API. In the old API, >> about 90% of the work was done in the `resolve()`, >> `_reverse_with_prefix()` and `populate()` of the RegexURLResolver. This >> made for a tightly coupled design that was almost impossible to extend. >> >> My starting point was the RegexURLResolver and RegexURLPattern. Both >> have a regex constraint that can match the current path and extract >> arguments. The former can then pass the remainder of the path to its >> list of resolvers and patterns; the latter can return a ResolverMatch >> containing the callback specified by that pattern. I've kept this >> distinction, with the `Resolver` and `View` classes. The change of name >> from `Pattern` to `View` is because the `View` object has a bit more >> logic that determines how the view is called. It is a thin, callable >> wrapper that can optionally decorate the actual view function with a set >> of decorators passed down from parent resolvers, and when overwritten, >> can do some more processing before or after the view function is called. > > This is a minor point, but I don't believe that the name `View` works > for this class; it's too ambiguous when we already have a `View` class > at the root of the class-based-views hierarchy. This class is not a > view: it wraps one or calls one, but it is not itself the view. > > I'm also curious to learn more about this "set of decorators" that can > be applied to a view by the url resolver - that feature doesn't seem to > be documented in the linked gist or in the API example. > >> The hard-coded dependence on a regex pattern has been abstracted to a >> Constraint object. Each Resolver and View has a (set of) Constraint >> object(s), that can match the current url and extract arguments. A >> RegexPattern that simply mimics the old behaviour will be available, but >> other implementations, such as a Constraint that matches the request's >> host or method, are easily provided. A Constraint can also reverse a set >> of arguments to a partial url. That means that the full set of >> constraints used to match an url to a view, together with a suitable set >> of arguments, can be reversed to the url itself. > > I like the general idea of the Constraint object! It reminds me of > Pyramid's view predicates system, which I think are a good model that it > may be worth spending some time reviewing for inspiration: > http://docs.pylonsproject.org/projects/pyramid/en/latest/narr/viewconfig.html > >> The main strength of a Constraint is that it can contain very specific >> logic about its arguments. For example, a Constraint may know that it >> resolves to a Model's primary key. If it then receives a Model instance >> of that particular type, it will know how to reverse that model instance >> to a valid string-based partial url, so that it can later be resolved to >> match the same object. It could also e.g. infer the regex pattern from >> the field's type. > > I am not at all sure that it is a good idea to mix layers in this > suggested way, or what specific use case it improves. If the argument > that a constraint matches is an integer, why is it (in practice) useful > for the constraint to know that this integer is supposed to be the ID of > some particular model class? Why is it good for that constraint, when > reversing, to be able to take a model instance instead of an ID? To me > this sounds like the sort of implicit type coercion that makes APIs > harder to use and understand, not easier. > >> There's one final piece to the puzzle: the URL object. This is the state >> of the url in the process of resolving or reversing an url. It's a >> two-way street: when resolving, it starts out as a full path, and the >> Constraints chip away at the path, while the set of constraints and >> extracted argument grows. When reversing, it starts out as a set of >> constraints and arguments, and reconstructs the partial urls from those >> constraints and arguments until a full url path is reconstructed. It >> shifts some of the logic from the Resolver to the URL, so that it is >> easier to extend the Resolver. It is also a simple container that allows >> any Constraint access to the full request. Last but not least, it allows >> to dynamically build an url against the current request. This is useful >> if e.g. a constraint matches a different subdomain than the current >> request, so that a link automatically points to the right domain. > > In reviewing the code samples in the linked gist, the part that I found > most difficult to understand was the URL object. > > In part I think this is because it is mis-named (it is not really just a > url, it's a full request). > > It's also because of the mutable nature of the URL object; especially > the fact that it's not the URL object itself, but the Constraint > objects, which are responsible for mutating the URL as it passes through > them. It also seems that the concept of "stripping down" and "building > up" a mutable URL object is highly tied to the URL path specifically. > It's not at all clear how it would apply to other possible types of > Constraints, such as those based on hostname or method: is a > method-based Constraint supposed to somehow "strip" the method from the > URL object? > > I suspect that there are better design options here. In general, > immutable objects are easier to reason about and work with than mutable > ones, and my intuition says that a URL resolving system with the > properties you've described should be possible to implement in terms of > immutable objects. > > The mutability seems to mostly serve the specific use case of nested > path-based constraints, where the inner ones don't have to repeat the > initial part of the path from the outer ones. But I think this pattern > should be equally possible to implement by having the path prefix > information passed down to nested constraints at the time when the graph > is constructed, so that the fully-constructed Constraint knows about the > full path it is supposed to match, and actual matching doesn't require > passing a mutable URL through the graph. > >> I'm looking forwards to your feedback. > > Thanks again for seeking out feedback! I hope this feedback was useful; > I'd be happy to elaborate on anything that was unclear. > > Carl > > -- > You received this message because you are subscribed to the Google Groups > "Django developers (Contributions to Django itself)" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > Visit this group at http://groups.google.com/group/django-developers. > To view this discussion on the web visit > https://groups.google.com/d/msgid/django-developers/5547DEEE.5010000%40oddbird.net. > For more options, visit https://groups.google.com/d/optout. -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. Visit this group at http://groups.google.com/group/django-developers. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/39D4D970-A66F-445A-9A6E-1A62CB95C311%40polytechnique.org. For more options, visit https://groups.google.com/d/optout.
