Christian Boos wrote:
> Hello Alec,
>
> On Oct 30, 5:45 pm, "Alec Thomas" <[EMAIL PROTECTED]> 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
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
context-refactoring: removed `Resource.from_spec` in favor of an improved
Resource() constructor call, based on `Resource.__new__`.
We also put the emphasis here on using `False` for the default argument for
`id` and `version`.
This is especially important for `version`, where an explicit `None` value can
be given, in order to specify the "latest" version.
diff -r 95d92e41b15e trac/mimeview/api.py
--- a/trac/mimeview/api.py Tue Oct 30 19:48:51 2007 +0100
+++ b/trac/mimeview/api.py Wed Oct 31 14:07:52 2007 +0100
@@ -116,11 +116,11 @@ class Context(object):
generated output to "authorized" informations only
"""
self.parent = None #: The parent context, if any
- self.resource = Resource.from_spec(resource)
+ self.resource = resource
self.href = href
self.perm = perm
- def from_request(cls, req, resource=None, id=None, version=None,
+ def from_request(cls, req, resource=None, id=False, version=False,
absurls=False):
"""Create a rendering context from a request.
@@ -151,8 +151,8 @@ class Context(object):
else:
href = None
perm = None
- self = cls(Resource.from_spec(resource, id=id, version=version),
- href=href, perm=perm)
+ self = cls(Resource(resource, id=id, version=version), href=href,
+ perm=perm)
self.req = req
return self
from_request = classmethod(from_request)
@@ -166,7 +166,7 @@ class Context(object):
context = context.parent
return '<%s %s>' % (type(self).__name__, ' - '.join(reversed(path)))
- def __call__(self, resource, id=None, version=None):
+ def __call__(self, resource, id=False, version=False):
"""Create a nested rendering context.
`self` will be the parent for the new nested context.
@@ -183,12 +183,10 @@ class Context(object):
>>> ticket1 = Resource('ticket', 1)
>>> context('ticket', 1).resource == ticket1
True
- >>> context(ticket1).resource == ticket1
- True
- >>> context(ticket1).resource == ticket1
+ >>> context(ticket1).resource is ticket1
True
"""
- resource = Resource.from_spec(resource, id=id, version=version)
+ resource = Resource(resource, id=id, version=version)
context = Context(resource, href=self.href, perm=self.perm)
context.parent = self
@@ -197,7 +195,7 @@ class Context(object):
# object being available, but that will hopefully improve in the
# future
if hasattr(self, 'req'):
- context.req = self. req
+ context.req = self.req
return context
diff -r 95d92e41b15e trac/mimeview/tests/api.py
--- a/trac/mimeview/tests/api.py Tue Oct 30 19:48:51 2007 +0100
+++ b/trac/mimeview/tests/api.py Wed Oct 31 14:07:52 2007 +0100
@@ -11,11 +11,13 @@
# individuals. For the exact contribution history, see the revision
# history and logs, available at http://trac.edgewall.org/log/.
+import doctest
import unittest
from StringIO import StringIO
from trac.core import *
from trac.test import EnvironmentStub
+from trac.mimeview import api
from trac.mimeview.api import get_mimetype, IContentConverter, Mimeview, \
_group_lines
from genshi import Stream, Namespace
@@ -210,6 +212,7 @@ class GroupLinesTestCase(unittest.TestCa
def suite():
suite = unittest.TestSuite()
+ suite.addTest(doctest.DocTestSuite(api))
suite.addTest(unittest.makeSuite(GetMimeTypeTestCase, 'test'))
suite.addTest(unittest.makeSuite(MimeviewTestCase, 'test'))
suite.addTest(unittest.makeSuite(GroupLinesTestCase, 'test'))
diff -r 95d92e41b15e trac/perm.py
--- a/trac/perm.py Tue Oct 30 19:48:51 2007 +0100
+++ b/trac/perm.py Wed Oct 31 14:07:52 2007 +0100
@@ -447,11 +447,11 @@ class PermissionCache(object):
def _normalize_resource(self, realm_or_resource, id, version):
if realm_or_resource:
- return Resource.from_spec(realm_or_resource, id, version)
+ return Resource(realm_or_resource, id, version)
else:
return self._resource
- def __call__(self, realm_or_resource, id=None, version=None):
+ def __call__(self, realm_or_resource, id=False, version=False):
"""Convenience function for using thus:
'WIKI_VIEW' in perm(context)
or
@@ -460,11 +460,11 @@ class PermissionCache(object):
'WIKI_VIEW' in perm(resource)
"""
- resource = Resource.from_spec(realm_or_resource, id, version)
+ resource = Resource(realm_or_resource, id, version)
return PermissionCache(self.env, self.username, resource, self._cache)
- def has_permission(self, action, realm_or_resource=None, id=None,
- version=None):
+ def has_permission(self, action, realm_or_resource=None, id=False,
+ version=False):
resource = self._normalize_resource(realm_or_resource, id, version)
return self._has_permission(action, resource)
@@ -485,7 +485,7 @@ class PermissionCache(object):
return decision
__contains__ = has_permission
- def require(self, action, realm_or_resource=None, id=None, version=None):
+ def require(self, action, realm_or_resource=None, id=False, version=False):
resource = self._normalize_resource(realm_or_resource, id, version)
if not self._has_permission(action, resource):
raise PermissionError(action, resource)
diff -r 95d92e41b15e trac/resource.py
--- a/trac/resource.py Tue Oct 30 19:48:51 2007 +0100
+++ b/trac/resource.py Wed Oct 31 14:07:52 2007 +0100
@@ -93,15 +93,6 @@ class Resource(object):
the real work to the Resource's manager.
"""
- def __init__(self, realm, id=None, version=None, parent=None):
- """Create a resource identifier."""
- if not isinstance(realm, basestring):
- raise TypeError("realm must be a string, got " + repr(realm))
- self.realm = realm
- self.id = id
- self.version = version
- self.parent = parent
-
def __repr__(self):
path = []
r = self
@@ -132,52 +123,68 @@ class Resource(object):
# -- methods for creating other Resource identifiers
- def from_spec(cls, resource_or_realm, id=None, version=None):
+ def __new__(cls, resource_or_realm=False, id=False, version=False,
+ parent=False):
"""Create a new Resource object from a specification.
- :param spec: this can be one of the following:
- - a `basestring`, which can be used to specify a `realm`
- - a `tuple`, which can be used to specify either a
- resource `(realm,id)` or a specific version of a
- resource `(realm,id,version)`
- - `''` or `None`, which can be used to specify the
- whole environment
+ :param resource_or_realm: this can be either:
+ - a `Resource`, which is then used as a
+ base for making a copy
+ - a `basestring`, used to specify a `realm`
+ :param id: the resource identifier
+ :param version: the version or `None` for indicating the latest version
+
+ If `id` is overriden, then the original `version` value will not be
+ reused.
+
+ >>> main = Resource('wiki', 'WikiStart')
+ >>> repr(main)
+ "<Resource u'wiki:WikiStart'>"
+
+ >>> Resource(main) is main
+ True
+
+ >>> repr(Resource(main, version=3))
+ "<Resource u'wiki:[EMAIL PROTECTED]'>"
+
"""
+ realm = resource_or_realm
if isinstance(resource_or_realm, Resource):
- return resource_or_realm
- if isinstance(resource_or_realm, tuple):
- version = None
- if len(resource_or_realm) == 3:
- realm, id, version = resource_or_realm
- elif len(resource_or_realm) == 2:
- realm, id = resource_or_realm
+ if (id, version, parent) == (False, False, False):
+ return resource_or_realm
+ else: # copy and override
+ realm = resource_or_realm.realm
+ if id is False:
+ id = resource_or_realm.id
+ if version is False:
+ if id == resource_or_realm.id:
+ version = resource_or_realm.version # could be 0...
+ else:
+ version = None
+ if parent is False:
+ parent = resource_or_realm.parent
else:
- realm = resource_or_realm
- return Resource(realm or '', id, version)
- from_spec = classmethod(from_spec)
+ if id is False:
+ id = None
+ if version is False:
+ version = None
+ if parent is False:
+ parent = None
+ resource = super(Resource, cls).__new__(cls)
+ resource.realm = realm
+ resource.id = id
+ resource.version = version
+ resource.parent = parent
+ return resource
+
def __call__(self, realm=False, id=False, version=False, parent=False):
"""Create a new Resource using the current resource as a template.
Optional keyword arguments can be given to override `id` and
`version`.
- If `realm` is changed, then the original `id` and `version` values
- will not be reused.
- If `id` is changed, then the original `version` value will not be
- reused.
"""
- if realm is False: # i.e. not set, so we stay in the same realm
- realm = self.realm
- if realm != self.realm:
- return Resource(realm or '', id or None, version or None, parent)
- else:
- if id is False:
- id = self.id
- if version is False:
- version = id == self.id and self.version or None
- if parent is False:
- parent = self.parent
- return Resource(realm, id, version, parent)
+ return Resource(realm is False and self or realm, id, version, parent)
# -- methods for retrieving children Resource identifiers
@@ -265,7 +272,7 @@ def get_resource_url(env, resource, href
'/trac.cgi/generic/Main?action=diff&version=5'
"""
- manager = ResourceSystem(env).get_resource_manager(resource.realm)
+ manager = ResourceSystem(env).get_resource_manager(resource.realm)
if not manager or not hasattr(manager, 'get_resource_url'):
args = {'version': resource.version}
args.update(kwargs)
