[PATCH]Make it easier to override getSubmodel

[twisted-trac]

	@jyknight reported
Trac ID	trac#316
Type	defect
Created	2003-10-06 22:46:18Z

Searchable metadata

``` trac-id__316 316 type__defect defect reporter__jknight jknight priority__high high milestone__ branch__ branch_author__ status__closed closed resolution__fixed fixed component__ keywords__ time__1065480378000000 1065480378000000 changetime__1076053203000000 1076053203000000 version__ owner__dp dp cc__dp cc__jknight ```

[twisted-trac]

	dp commented

#!html
<pre>
As I said in that irc conversation, I believe the correct way to solve this problem is to make 
getSubmodel work like submodelFactory does right now and deprecate the use of 
submodelFactory.
</pre>

[twisted-trac]

	@jyknight commented

#!html
<pre>
The docs don't seem to mention anything about "submodelFactory" but do say that you want to 
override "getSubmodel". However, it seems as if there is no reason you ever really do want to 
override getSubmodel as if you do you cannot return a Deferred.
</pre>

[twisted-trac]

	@moshez commented

#!html
<pre>
they are different.
getSubmodel returns a model, submodelFactory returns something
which gets turned into a model. They are different.
You usually want to override neither, you want to use MethodModel
or AttributeModel. If you don't, then you probably should override
from getSubmodel and just use adaptToIModel if necessary.

I don't think this is a bug. "getSubmodel", by its very name,
should return a model.
</pre>

[twisted-trac]

	@jyknight commented

#!html
<pre>
Doc bugs are still bugs. I realize the methods are different (now) -- my issue is that the 
documentation did not tell me that, or even tell me that submodelFactory existed.

Also:

[11:18] &lt;foom> is there any reason to ever override getSubmodel instead of submodelFactory?
[11:19] &lt;glyph> foom: nope
[11:19] &lt;foom> okay, then there's a doc bug
[11:20] &lt;fzZzy> foom: basically submodelFactory should be renamed getSubmodel and the code 
in getSubmodel should be moved higher up into the framework
[11:20] &lt;glyph> foom: one among many :-\.  fzZzy and I have been discussing this rather 
systematic problem of woevn for the last day or two
</pre>

[twisted-trac]

	@jyknight commented

#!html
<pre>
Ah, I thought that suggestion was a "I wish it were so, good thing to change for woven2" type of 
suggestion. If it's possible to do so before a total rewrite, please retitle the bug. Otherwise, 
I think the docs should be fixed.
</pre>

[twisted-trac]

	dp commented

#!html
<pre>
It should be pretty easy to do without a total rewrite. Also, woven2 will probably not be as much of 
a total rewrite as a refactoring with a chainsaw on fire.

Anyway, thanks for raising it, I'll try to find time to get this in as soon as possible.
</pre>

[twisted-trac]

	@moshez commented

#!html
<pre>
The following patch looks like it does the right thing:
Index: twisted/web/woven/model.py
===================================================================
RCS file: /cvs/Twisted/twisted/web/woven/model.py,v
retrieving revision 1.51
diff -u -r1.51 model.py
--- twisted/web/woven/model.py  6 Oct 2003 15:10:17 -0000       1.51
+++ twisted/web/woven/model.py  24 Oct 2003 17:30:49 -0000
@@ -170,7 +170,7 @@
             elif element == '..':
                 currentModel = currentModel.parent
             else:
-                currentModel = currentModel.getSubmodel(request, element)
+                currentModel = currentModel._getSubmodel(request, element)
                 if currentModel is None:
                     return None
         return currentModel
@@ -200,11 +200,15 @@
         Deferred, then I ought to check for cached values (created by
         L{setSubmodel}) before doing a regular Deferred lookup.
         """
+        return self.submodelFactory(request, name)
+
+
+    def _getSubmodel(self, request, name):
         if self.submodels.has_key(name):
             return self.submodels[name]
         if not self.submodelCheck(request, name):
             return None
-        m = self.submodelFactory(request, name)
+        m = self.getSubmodel(request, name)
         if m is None:
             return None
         sm = adaptToIModel(m, self, name)
</pre>

[twisted-trac]

	@moshez commented

#!html
<pre>
This patch breaks woven unit tests.
I'll try to work on a better patch, in the mean time I'm removing the "patch"
tag.
</pre>

[twisted-trac]

	@moshez commented

#!html
<pre>
OK, I've attached a newer patch that manages to pass unit tests.
However, I'm starting to think maybe this should be just a doc issue
"you sometimes wanna override submodelFactory".
Donovan, please either apply or reject, and in any way close this bug.
</pre>

[twisted-trac]

	@jyknight commented

#!html
<pre>
I think you forgot to attach it?
</pre>

[twisted-trac]

	@jyknight commented

#!html
<pre>
This bug is being resolved, wontfix, as woven is "almost deprecated" and unmaintained. Use nevow 
instead. See &lt;http://divmod.org/users/dp.twistd/wiki/> and the twisted-web mailing list &lt;http://
twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-web>.
</pre>