Issue 6453060: Registry improvements (Closed)
DescriptionRegistry improvements
Patch Set 1 #
Total comments: 7
Patch Set 2 : Alter a few comments, changed several __call__ signatures, add missing @register for NonExistent #
Total comments: 11
Patch Set 3 : Fix spacing, remove auto_parenthesis, converter registry #
Total comments: 12
Patch Set 4 : remove decorators #
MessagesTotal messages: 9
See my comments in the files :) http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/__init__.py File MoinMoin/items/__init__.py (right): http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:83: return self.priority < other.priority This fixes https://bitbucket.org/thomaswaldmann/moin-2.0/issue/221 http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:102: register = add_auto_parenthesis(item_registry.register, TypeType) It could be just register = item_register.register, but adding auto-parenthesis makes code reads better (though it makes things a bit more implicit). http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/__init__.py#newco... MoinMoin/items/__init__.py:549: itemtype = u'default' itemtype is now a class property. Same thing for content_type properties in Content subclasses. http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/content.py File MoinMoin/items/content.py (right): http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/content.py#newcod... MoinMoin/items/content.py:108: def register(self, priority=RegistryBase.PRIORITY_MIDDLE): The priority member is inherited from old code, it is actually not used anywhere now. I put priority in the decorator factory rather than as a class property because i think it's not intrinsic of the class, but rather selected at time of registering. For instance, one might register Binary (contenttype */*) once with normal priority (which puts it at the end) and the second time with high priority (which puts it at the beginning). However, I can find practical use cases of neither `priority` itself nor registering a classes several times with different priorities. Justification needed. If it's a historical leftover (or some premature engineering), I would suggest to remove it altogether. Removing priority has one more advantage - it will also obsolete the decorator factory which `register` current is. (After removing priority `register_class` can be just renamed to `register`, and the auto_parenthesis trick is not needed either.) http://codereview.appspot.com/6453060/diff/1/MoinMoin/items/content.py#newcod... MoinMoin/items/content.py:442: class ApplicationXGTar(ApplicationXTar): Such subclassing is required since it is now assumed that each Content class has one and only one contenttype. It is also consistent with how MoinWiki, CreoloText, etc. subclass Text. http://codereview.appspot.com/6453060/diff/1/MoinMoin/util/decorator.py File MoinMoin/util/decorator.py (right): http://codereview.appspot.com/6453060/diff/1/MoinMoin/util/decorator.py#newco... MoinMoin/util/decorator.py:16: that @wrapped with no parameter list is equivalent to @wrapped(). IIRC such practice is fairly common, although I cannot come up with an example of such use in popular libraries now... http://codereview.appspot.com/6453060/diff/1/MoinMoin/util/registry.py File MoinMoin/util/registry.py (right): http://codereview.appspot.com/6453060/diff/1/MoinMoin/util/registry.py#newcode46 MoinMoin/util/registry.py:46: conv = entry(*args, **kw) This modification and addition of Entry.__call__ comes from how it is done in items.content.RegistryContent and converters.RegistryConverter. Consequently RegistryContent.get and RegistryConverter.get are not needed anymore. It doesn's make sense if you provide a lame default and override that to exactly the same thing (well, almost exactly) in all subclasses. :)
Sign in to reply to this message.
nice cleanup. some minor comments about consistent spacing. and think about whether that auto_parenthesis stuff is worth it. http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py File MoinMoin/items/content.py (right): http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:255: remove empty line http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:268: remove ... http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:699: remove the empty line http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:898: remove empty line http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:962: remove... http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:1030: remove... http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py File MoinMoin/util/decorator.py (right): http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py#ne... MoinMoin/util/decorator.py:16: that @wrapped with no parameter list is equivalent to @wrapped(). is this worth having this strange function (and extra code/module) and all the calls to it everybody is wondering about at first? :) http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py#ne... MoinMoin/util/decorator.py:34: case, you can work around by using a keyword argument instead. ... especially considering this.
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py File MoinMoin/items/content.py (right): http://codereview.appspot.com/6453060/diff/6001/MoinMoin/items/content.py#new... MoinMoin/items/content.py:255: On 2012/07/29 12:22:31, ThomasJWaldmann wrote: > remove empty line fixing... http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py File MoinMoin/util/decorator.py (right): http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py#ne... MoinMoin/util/decorator.py:16: that @wrapped with no parameter list is equivalent to @wrapped(). On 2012/07/29 12:22:31, ThomasJWaldmann wrote: > is this worth having this strange function (and extra code/module) and all the > calls to it everybody is wondering about at first? :) yeah, i actually hesitated. writing @register() is actually okay for me, especially considering that explicit is better than implicit. but since i remember having seen this auto-parenthesis trick before in several places (unfortunately i can't name them for now) i wonder if anybody would like it so i took it as an exercise and put it here. :) also, please see my comments in content.py of patch set 1 http://codereview.appspot.com/6453060/patch/1/3. if we remove the priority parameter, we can just write decorators instead of decorator functions.
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py File MoinMoin/util/decorator.py (right): http://codereview.appspot.com/6453060/diff/6001/MoinMoin/util/decorator.py#ne... MoinMoin/util/decorator.py:16: that @wrapped with no parameter list is equivalent to @wrapped(). On 2012/07/29 14:16:35, xiaq wrote: > On 2012/07/29 12:22:31, ThomasJWaldmann wrote: > > is this worth having this strange function (and extra code/module) and all the > > calls to it everybody is wondering about at first? :) > > yeah, i actually hesitated. writing @register() is actually okay for me, > especially considering that explicit is better than implicit. but since i > remember having seen this auto-parenthesis trick before in several places > (unfortunately i can't name them for now) i wonder if anybody would like it so i > took it as an exercise and put it here. :) > > also, please see my comments in content.py of patch set 1 > http://codereview.appspot.com/6453060/patch/1/3. if we remove the priority > parameter, we can just write decorators instead of decorator functions. s/functions/factories/
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (right): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:46: # TODO Maybe make this a decorator factory, as in items/__init__.py and Converter are often registered multiple times. To turn register() into @register we can either 1. support specifying type_input and type_output as lists. eg: @register class Converter: type_input = ['application/a', 'application/b'] type_output = 'text/moin' 2. subclass the converter for each registration, eg. class Converter(object): ... class Converter1(Converter): type_input = 'application/a' type_output = 'text/moin' class Converter2(Converter): type_input = 'application/b' type_output = 'text/moin' I've recorded the all converter registration and pasted in [1]. It is observed that (except for everything.in) only in_* converters are registered multiple times, and type_output is invariant on each registration. This is expected anyway. :) It is also observed that some in_converters are registered multiple times because there are synonyms of type_input, like 'text/x.moin.creole' and 'x-moin/format;name=creole'; while others are not, only that they happen to be processable by the same converter, like 'application/vnd.oasis.opendocument.chart' and 'application/vnd.oasis.opendocument.database'. Perhaps it is appropriate to do 1. or 2. depending on whether the contenttypes are synonyms? 1. http://dpaste.com/777781/
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (right): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:46: # TODO Maybe make this a decorator factory, as in items/__init__.py and On 2012/07/30 03:37:15, xiaq wrote: > Converter are often registered multiple times. To turn register() into @register > we can either > > 1. support specifying type_input and type_output as lists. eg: > > @register > class Converter: > type_input = ['application/a', 'application/b'] > type_output = 'text/moin' > This in fact can be done in a smarter way. Since contenttype never contain whitespaces, type_input and type_output can be a whitespace-delimited list. (Hint: if this sounds a bit too smart, loot at collections.namedtuple of stdlib). > 2. subclass the converter for each registration, eg. > > class Converter(object): > ... > > class Converter1(Converter): > type_input = 'application/a' > type_output = 'text/moin' > > class Converter2(Converter): > type_input = 'application/b' > type_output = 'text/moin' > > I've recorded the all converter registration and pasted in [1]. It is observed > that (except for everything.in) only in_* converters are registered multiple > times, and type_output is invariant on each registration. This is expected > anyway. :) > > It is also observed that some in_converters are registered multiple times > because there are synonyms of type_input, like 'text/x.moin.creole' and > 'x-moin/format;name=creole'; while others are not, only that they happen to be > processable by the same converter, like > 'application/vnd.oasis.opendocument.chart' and > 'application/vnd.oasis.opendocument.database'. > > Perhaps it is appropriate to do 1. or 2. depending on whether the contenttypes > are synonyms? > > 1. http://dpaste.com/777781/
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (left): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:65: def get(self, type_input, type_output, **kw): Why? http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (right): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:29: class Entry(namedtuple('Entry', 'factory type_input type_output priority')): "The field_names are a sequence of strings such as ['x', 'y']" http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:30: def __call__(self, type_input, type_output, **kw): Why do you change the method signature? http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:37: if self.priority != other.priority: This is a change in behaviour. Please explain. http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:46: # TODO Maybe make this a decorator factory, as in items/__init__.py and The type is an object, not a string. This comment does not relate to the rest of the change. Split them.
Sign in to reply to this message.
http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (left): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:65: def get(self, type_input, type_output, **kw): On 2012/07/31 09:09:35, waldi wrote: > Why? see http://codereview.appspot.com/6453060/patch/11001/12004. http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.py File MoinMoin/converter/__init__.py (right): http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:29: class Entry(namedtuple('Entry', 'factory type_input type_output priority')): On 2012/07/31 09:09:35, waldi wrote: > "The field_names are a sequence of strings such as ['x', 'y']" "Alternatively, field_names can be a single string with each fieldname separated by whitespace and/or commas, for example 'x y' or 'x, y'." http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:30: def __call__(self, type_input, type_output, **kw): On 2012/07/31 09:09:35, waldi wrote: > Why do you change the method signature? this is to accommodate RegistryBase.get, which passes arguments to __call__ as *args, **kw. See http://codereview.appspot.com/6453060/patch/11001/12004. RegistryConvert.get, RegistryContent.get and RegistryItem.get are gone now. http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:37: if self.priority != other.priority: On 2012/07/31 09:09:35, waldi wrote: > This is a change in behaviour. Please explain. https://bitbucket.org/thomaswaldmann/moin-2.0/issue/221/registrycontententry-... Applies here as well. http://codereview.appspot.com/6453060/diff/11001/MoinMoin/converter/__init__.... MoinMoin/converter/__init__.py:46: # TODO Maybe make this a decorator factory, as in items/__init__.py and On 2012/07/31 09:09:35, waldi wrote: > The type is an object, not a string. > yes, but we can make it a string and registry with Entry(cls._factory, Type(cls.type_input), Type(cls.type_output), priority). See the contenttype class property present in http://codereview.appspot.com/6453060/patch/11001/12003. > This comment does not relate to the rest of the change. Split them.
Sign in to reply to this message.
I've removed the decorators due to waldi's request to keep the patch clean. Also i'm holding back the decorator idea a bit since it's nontrivial to convert the Converter's to use decorators, and having items and items.content registries use decorators but converters not introduces inconsistency.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
