Hi, A few updates on the topic, as I really want to get through this. I'm willing to make changes as needed, but for that, I first need to better understand cmlenz concerns.
Christian Boos wrote: > Christopher Lenz wrote: > >> [...] >> self_href() is an *awful* name. Even after reading the docstring for >> a couple of times, I still don't really get why it's there and what >> it does. Looking at the invocations, there's apparently only *one* >> place that actually passes an argument to it, and that's in >> formatter.py. >> > > Agreed, I already wanted to change that a few times. 'resource_href' > would probably be more appropriate. > ... resource_href ... is used to produce the "canonical" URL for > accessing the resource that the context describes. > Fixed in http://trac.edgewall.org/changeset/4666/sandbox/security >> Why is local_url() defined on the Context class? It's only used by >> formatter.py, so why not keep it there? It's just about how external >> links are rendered, it shouldn't concern anyone outside the formatter. >> >> > > Yes, I originally thought it could have more uses, but finally it > didn't, so I think we can put that back in the formatter. > Also done in r4666. >> What's the reason for moving the DB connection handle from the >> formatter to the context? >> >> > > Because you often use a Context without a Formatter object, but when you > use the formatter, you always have a Context. > I need to think a bit further on this one, particularly w.r.t. the impact on the db connection pool, as this change could lead to retain the connections outside of the pool for a longer period of time than necessary. >> And a more fundamental kind of question: is it really necessary that >> the Context knows the req (and abs_urls, by extension)? I.e. why must >> a Context be able to produce links? Couldn't we just have continued >> to pass the req to e.g. render_macro() in addition to the context? I >> feel like I'm missing a fundamental point here. >> >> > > I think so. How would a macro know that it's called from within a RSS > output or an regular HTML page? IOW, how would it know that it has to > use req.abs_href or req.href? Before, we used to check for format == > 'rss' everywhere, but that was more a hack... Now macros can use > context.href (or formatter.href, but it's the same). > ... and btw, that's what we used to do with the Formatter since the beginning. The context is only a bit more general than the Formatter. Now of course there could have been other approaches, with different trade-offs (e.g. having RenderingContext and ResourceDescriptor classes). The point is that I don't think it's necessarily a good idea to strive for an hypothetical "perfect" solution and the "ideal" design... The current approach is already more powerful than what we had before, and nothing prevents anyone to propose a better scheme *later*. Of course, if deficiencies are clearly pointed out now, I'm willing to address them, but please provide me with some concrete topics where I can act upon. Also, this is not (should not be) only a discussion between cmlenz and me. Others opinions would be helpful. -- Christian --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Trac Development" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [EMAIL PROTECTED] For more options, visit this group at http://groups.google.com/group/trac-dev?hl=en -~----------~----~----~----~------~----~------~--~---
