context-refactoring branch ready for merge
Christian Boos
After much debate and tweaking, the context-refactoring should now be in
a good shape for merging on trunk.
For those who have missed the (sometimes heated) discussions of the past
months, here's what it is about. During 0.11dev, I introduced a
`Context` class which was meant to hold all the necessary information
for rendering "content". To that end, this object encapsulated the
commonly used env, req and db objects. A context was also indicating
which Trac "resource" was being worked on, by the way of .realm, .id and
.version properties. There were a few methods that could be used to
describe that resource as well, like for getting the name or the url of
that resource. In addition, contexts could be nested (have a parent
context). Finally, the fine grained permission scheme introduced in
0.11dev was using the context in order to grant different permissions
for individual resources.
Now there were some concerns that this design was a bit crufty, and I
agreed to rework this into something more consensual: a Context class
focusing on the rendering aspects and a Resource class focusing on the
identification of the resource.
The context doesn't contain unnecessary information anymore (no env, no
db) and only keeps the req because at that point there's no way to do
without. The longer term goal (hopefully 0.12) is to have a rendering
layer _hence the Context_ be completely independent from the web Request
object. The context is still associated to a resource by the way of a
.resource property, which is a Resource object, so that among other
things, relative links in the rendered wiki text can work.
A resource object bundles together .realm (like 'wiki', 'ticket', etc.),
.id (unique identifier within a realm) and .version (a number or None
for the latest), as well as a .parent resource in case of dependent
resources (typical and only example for now, the resources in the
'attachment' realm). There's no way to get to the underlying data model
object implicitly; if you need it, you'll have to instantiate explicitly
the appropriate model class for the given resource. Also, all the helper
methods for getting the url, name etc. of a resource in a generic way
have been replaced by helper functions taking an env and a resource
(there are more convenient name_of(resource, url_of(resource) helper
functions available in the templates, though). Finally, the fine grained
permission scheme is now using exclusively the resources for doing the
permission checks. A typical fined grained permission check can now be
done by writing something like:
req.perm(ticket.resource).require('TICKET_VIEW')
where ticket is the model object for some ticket.
There's no trac.context module anymore. The new Context class is now
located in trac.mimeview. The new Resource class, along with the
ResourceNotFound exception, IResourceManager interface and the
ResourceSystem, is located in trac.resource.
The full set of changes can be examined using the following diff URL,
but be warned that this is a somewhat big set of changes...
Things are certainly not yet 100% polished at this point, but the branch
did receive considerable attention, so almost everything should work (or
is it's a bug ;-) ).
Now, once that branch is in trunk, the timeline refactoring will follow
(I'll write a second mail about that one, but it should be much less
contentious) and after the most critical remaining bugs for 0.11 are
committed as well (#5025 and #2048), I propose that we release a
0.11beta1 and upgrade t.e.o to it. The advantage is that this will
provide some clarity when talking about the 0.11dev API: pre-beta1 will
be the old trac.context.Context and trac.timeline.TimelineEvent APIs,
and post-beta1 will be the new trac.resource.Resource and
trac.mimeview.Context ones. Therefore, a plugin would be either "beta1"
compatible or not.
-- Christian
Emmanuel Blot
Sorry if they really are; at least, I would have learnt more about
these controversial APIs ;-)
Quick question related to the context thing: what about the formatter,
especially in macros?
The method signature for a macro is (still):
def expand_macro(self, formatter, name, content)
I'm not sure to understand why we need a "formatter" to retrieve the resource.
With the current -trunk- implementation, a macro needs to retrieve the
resource this way: formatter.context.realm and formatter.context.id if
I'm not mistaken.
It seems fair that the formatter and the context are tied (although I
would have first expected the context to provide the formatter rather
than the opposite but I know really little about these APIs), but I
don't think the formatter should provide the resource, they should not
be directly related IMHO.
The method signature could be something like:
def expand_macro(self, formatter, resource, name, args)
couldn't it?
("content" does not sound as a good symbol for the macro args BTW)
Are you sure about the revision numbers in the above URL? For example,
the Context class in trac/mimeview/api.py does not appear, as it has
been committed in 6116.
[OT]: why a difference between two identical paths and revision gives
something but null ? Maybe it's due to the SVN duality between
revisions and changes, but it's really confusing. I would have
expected: old_rev=6116, new_rev=6117
Christian Boos
> Maybe the following ideas sounds crazy or stupid.
> Sorry if they really are; at least, I would have learnt more about
> these controversial APIs ;-)
>
> Quick question related to the context thing: what about the formatter,
> especially in macros?
>
> The method signature for a macro is (still):
> def expand_macro(self, formatter, name, content)
>
> I'm not sure to understand why we need a "formatter" to retrieve the resource.
> With the current -trunk- implementation, a macro needs to retrieve the
> resource this way: formatter.context.realm and formatter.context.id if
> I'm not mistaken.
>
> It seems fair that the formatter and the context are tied (although I
> would have first expected the context to provide the formatter rather
> than the opposite but I know really little about these APIs), but I
> don't think the formatter should provide the resource, they should not
> be directly related IMHO.
>
The formatter is tied to a rendering context and not the other way
around because not every mimeview rendering involves formatting wiki
text. Retrieving the resource information from the formatter can be done
either indirectly through the context (formatter.context.resource) or
directly, formatter.resource (it's the same thing). The formatter knows
about the resource to which the text being formatted belongs to, so that
relative links in that wiki content will use that resource as their
reference, so I actually think their directly related.
> The method signature could be something like:
> def expand_macro(self, formatter, resource, name, args)
> couldn't it?
>
That's a possibility, and if this clarifies things I'm OK to add that
(though that will probably break the macro API once again...).
> ("content" does not sound as a good symbol for the macro args BTW)
>
Well, macros could also be called using the block syntax, so in that
case at least it makes sense to talk about content. "content" is also a
more general term than arguments, and not every macro interpret their
content as "arguments" in the sense of comma-separated arguments.
>
>> http://trac.edgewall.org/anydiff?new_path=%2Fsandbox%2Fcontext-refactoring&old_path=%2Fsandbox%2Fcontext-refactoring&new_rev=6117&old_rev=6117
>>
>
> Are you sure about the revision numbers in the above URL? For example,
> the Context class in trac/mimeview/api.py does not appear, as it has
> been committed in 6116.
>
At the last minute, I didn't want to put a link to the diff itself but
rather to the form leading to the diff (to spare a few costly hits on
t.e.o). I forgot to edit the link, it should have been:
(r6113 was the last merge revision from trunk to the context-refactoring
branch)
> [OT]: why a difference between two identical paths and revision gives
> something but null ? Maybe it's due to the SVN duality between
> revisions and changes, but it's really confusing. I would have
> expected: old_rev=6116, new_rev=6117
When you specify old_rev==new_rev, it's like looking at that changeset.
-- Christian
Alec Thomas
Fantastic work cleaning this stuff up, looks like it was a lot of work.
I've only looked at the Resource class itself, and have some comments:
- In general I think having four different ways of creating resources
is both redundant and confusing:
- Resource(realm, id, version)
- Resource.from_spec(realm, id, version)
- Resource.from_spec(resource)
- Resource.from_spec((realm, id, version)).
This is extreme overkill IMO.
The tuple form in particular can be replaced with argument expansion
(*mytuple).
From looking at the usage of from_spec() it seems that the sole
reason for its existence is as a convenience so you can pass in a
resource *or* a realm and have a Resource object returned.
Is it a big deal if we put this logic into the constructor? The only
difference will be that it will always return a new object rather
than sometimes returning the resource object passed in.
This would leave us with:
- Resource(realm, id, version)
- Resource(resource)
Which I think is a lot cleaner and simpler.
- I'd prefer it if Resource.__call__() was renamed to clone, duplicate or
copy or something.
Alec
--
Evolution: Taking care of those too stupid to take care of themselves.
Christian Boos
On Oct 30, 5:45 pm, "Alec Thomas" <a...@swapoff.org> wrote:
> Hey Christian,
>
> Fantastic work cleaning this stuff up, looks like it was a lot of work.
>
> I've only looked at the Resource class itself, and have some comments:
>
> - In general I think having four different ways of creating resources
> is both redundant and confusing:
>
> - Resource(realm, id, version)
> - Resource.from_spec(realm, id, version)
> - Resource.from_spec(resource)
> - Resource.from_spec((realm, id, version)).
>
> This is extreme overkill IMO.
extreme overkill is perhaps itself an overstatement ;-)
>
> The tuple form in particular can be replaced with argument expansion
> (*mytuple).
We don't actually need the tuple form, that was just a temporary need.
I'll clean that up.
> From looking at the usage of from_spec() it seems that the sole
> reason for its existence is as a convenience so you can pass in a
> resource *or* a realm and have a Resource object returned.
Yeah, it's nothing more than that, used in both the Context and the
PermissionCache.
> Is it a big deal if we put this logic into the constructor? The only
> difference will be that it will always return a new object rather
> than sometimes returning the resource object passed in.
>
> This would leave us with:
>
> - Resource(realm, id, version)
> - Resource(resource)
Well, the only purpose of Resource.from_spec was to avoid the
Resource(resource) case and the creation of an unneeded object.
But there's a simple trick here to return resource itself in the
second case, so it might be worth doing it.
> Which I think is a lot cleaner and simpler.
>
> - I'd prefer it if Resource.__call__() was renamed to clone, duplicate or
> copy or something.
Any would be fine for me, like copy(). OK, then we need to do the same
on Context. I didn't start in that direction because I wanted to keep
some degree of backward compatibility with 0.11dev, but that
compatibility mode is now gone anyway, so...
-- Christian
Christian Boos
> Hello Alec,
>
> On Oct 30, 5:45 pm, "Alec Thomas" <a...@swapoff.org> wrote:
>
>> Is it a big deal if we put this logic into the constructor? The only
>> difference will be that it will always return a new object rather
>> than sometimes returning the resource object passed in.
>>
>> This would leave us with:
>>
>> - Resource(realm, id, version)
>> - Resource(resource)
>>
>
> Well, the only purpose of Resource.from_spec was to avoid the
> Resource(resource) case and the creation of an unneeded object.
> But there's a simple trick here to return resource itself in the
> second case, so it might be worth doing it.
>
See attached patch. Would that be OK?
>> Which I think is a lot cleaner and simpler.
>>
>> - I'd prefer it if Resource.__call__() was renamed to clone, duplicate or
>> copy or something.
>>
>
> Any would be fine for me, like copy(). OK, then we need to do the same
> on Context.
Actually, with the patch above, such a method wouldn't be needed
anymore, doing a copy + override can be a simple call to
Resource(resource, version=None), for example. But the change would be
rather involved, as I would need to go through all the resource() calls.
For context, it's a bit different, on second thought I think it's
actually better kept like this, as we have plenty of calls to e.g.
context('ticket', id) in the templates, which are nicer than e.g.
context.subcontext('ticket', id), I think.
-- Christian
Christopher Lenz
> After much debate and tweaking, the context-refactoring should now
> be in
> a good shape for merging on trunk.
+1
The branch is IMHO a vast improvement (design-wise) over what we
currently have in trunk. There are possibly still some places that
need spit and polish, but that can be applied on trunk.
Cheers,
Chris
--
Christopher Lenz
cmlenz at gmx.de
http://www.cmlenz.net/
Noah Kantrowitz
>
> Am 30.10.2007 um 11:04 schrieb Christian Boos:
>> After much debate and tweaking, the context-refactoring should now
>> be in
>> a good shape for merging on trunk.
>
> +1
>
> The branch is IMHO a vast improvement (design-wise) over what we
> currently have in trunk. There are possibly still some places that
> need spit and polish, but that can be applied on trunk.
+1 from me too. I can't say I'm still in love with the architecture,
but its moving in the right direction IMO and I don't see why it can't
be worked on in trunk.
--Noah
Christian Boos
> Am 30.10.2007 um 11:04 schrieb Christian Boos:
>
>> After much debate and tweaking, the context-refactoring should now
>> be in
>> a good shape for merging on trunk.
>>
>
> +1
>
> The branch is IMHO a vast improvement (design-wise) over what we
> currently have in trunk. There are possibly still some places that
> need spit and polish, but that can be applied on trunk.
>
>
Great! I'll merge it later today.
Following this, I'll branch a timeline-refactoring sandbox.
For the important remaining tickets, #5025 is already fixed in trunk and
there's a patch on #2048 for review (1).
There's still #5064, which has no patch yet but a proposal that I think
can be consensual (2): revert to the 0.10.4 behavior by default,
optionally allow enabling the 0.10.5dev behavior by a
"force_use_base_url_for_redirects" setting, for those who really need
it(?) (#2553 et al).
-- Christian
(1) http://trac.edgewall.org/attachment/ticket/2048/ticket2048-patches.diff
(2) http://trac.edgewall.org/ticket/5064#comment:18
Emmanuel Blot
So what's the status for the macro interface?
Cheers,
Manu
Christian Boos
>> Great! I'll merge it later today.
>>
>
> So what's the status for the macro interface?
>
Well, so far it's the status quo, documented here:
http://trac.edgewall.org/wiki/TracDev/ApiChanges/0.11#IWikiMacroProvider
The formatter argument provides the macro with all the needed contextual
information.
I'm not sure it will help a lot to augment the number of arguments in
the expand_macro method by flattening out the information already
contained in the formatter. Maybe make an exception for resource, but
what about perm, context, etc.?
As for the changes required in existing 0.11 plugins, it will be a
matter of replacing formatter.context.realm/id by
formatter.resource.realm/id.
-- Christian