commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bugzi...@apache.org
Subject DO NOT REPLY [Bug 35096] - [Digester][PATCH] Cannot add multiple CallMethodRules for the same pattern
Date Fri, 27 May 2005 21:03:22 GMT
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG·
RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT
<http://issues.apache.org/bugzilla/show_bug.cgi?id=35096>.
ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND·
INSERTED IN THE BUG DATABASE.

http://issues.apache.org/bugzilla/show_bug.cgi?id=35096





------- Additional Comments From akolkar@us.ibm.com  2005-05-27 23:03 -------
(In reply to comment #2)
> Hi Rahul,
> Thanks very much for offering a patch for this. It is always good to see 
people
> offering solutions rather than just problems :-).

I used Digester a couple of times recently, I found it quite nifty. Logically, 
the next step was to try to fix the next problem reported on the mailing list.

> I do have some concerns about this approach though, that mean I personally 
would
> prefer not to see this patch committed as-is. If you wished to work on it 
some
> more, I'd be happy to help. However CallMethodRule has acted this way since 
the
> very first release, so I don't see it being worthwhile to delay the next 
release
> to get this in. 

Sure, I can do multiple iterations based on feedback. Release descisions are 
upto the Digester's RM.

> Of course others may see it differently; if any other digester committers are
> listening, please speak up!
> As I note below, a user rule should be able to do anything a built-in rule 
can.
> So if this problem is solvable, then it should be solvable via a bunch of new
> Rule classes that can be downloaded separate from the commons-digester 
release.
> That would be a good thing I think as people can use them and give feedback
> before the new functionality becomes part of the digester core. I think it 
quite
> feasable to put such "extras" code up on the apache site somewhere for 
people to
> download.

Thats one approach. I, personally, would never bother to download any extras 
unless:
a) They were well-publicized
b) The problems they solve were clearly articulated
c) They solved a problem I was encountering
The revolutionary approach would be to demonstrate full backward 
compatibility, and include in the distro.

> The concept of a stack of arrays was, I think, rather ugly in the first 
place.
> Extending this by having a stack of lists of arrays isn't, in my opinion, the
> way to go.

OK, thats water under the bridge, here are the minimal requirements going 
forward:
1) First, allow a call method rule to create its scratch space
2) Allow subsequent call param rules to populate the appropriate scratch space
3) Finally, allow the call method rule to consume (and release) the scratch 
space
And this, without tripping on other call method rules (for the same pattern, 
or otherwise).

> And while it really will be necessary to somehow link CallParamRule objects 
to
> their associated CallMethodRule I don't think using integer indexes are the
> solution. It just feels clumsy.

I'm moderately surprised, primarily, due to the existence of paramIndex. I had 
a similar reaction when I saw paramIndex, since I don't specify where an 
argument should go on the parametes stack when making a function call, I just 
supply the arguments in the right order. So I wondered why that index exists, 
and figured out (as I wrote some Digesters), that one may want to order the 
addRules by patterns, and in complex schemas, there are times when you 
encounter params 'out of order' as you traverse the document structure. Now, 
the trouble is, this was designed as a one-dimensional space on the parameters 
stack in Digester (which is why it fails today), when it is, indeed, a two-
dimensional space. One index per dimension.

> Having any kind of rule treated "special" by the digester core is also
> undesirable. It is much nicer to have a core that just provides generic rule
> services, and then the rule classes are layered on top. Ok, apart from all 
those
> factory methods that are also on the Digester class. Having this layering 
means
> that a user rule can do anything that a built-in rule can. Currently this is
> true, and I'd like to see it stay that way.

My mistake, should have been more explicit in the TODO I added on top of the 
instanceof. I do not want any special treatment for CallMethodRule, the rogue 
bits in the core can be easily shifted to CallMethodRule. The purpose I put 
them there, was to socialize those and get feedback of the form "hey, here's a 
better way of figuring out this is the n'th callMethodRule added for pattern 
foo/bar". We can put that bit in CallMethodRule, the setDigester isn't exactly 
a side-effect free setter anyway.

> I was experimenting a while ago with the following:
> * 
> having an interface (MethodInvoker or somesuch name) that the CallMethodRule
> implements.
> *
> having XXParamRule objects obtain the Rules object from the digester and 
iterate
> over the list of Rule objects until they find themself. The immediately
> preceding MethodInvoker rule object is then the one that they associate 
themself
> with. This can be done when setDigester is called, ie once only.

This can always be the default behavior when the callMethodRule index is not 
specified. But again, I recommend that it be possible for a CallParamRule to 
point to the CallMethodRule that it is refering too (I used index in document 
order). If you buy paramIndex, this is a straight sell. Anytime we introduce a 
default behavior which cannot be overridden, we're just digging ourselves 
another hole. I recently had to call methods with interleaved params occuring 
in the document structure, ofcourse, I had to tackle it with custom rules. 
Which brings me to a broad comment, based on my use of Digester, in its 
current state, its very useful for flat schemas, other than that you end up 
writing a lot of custom rules.

> * 
> having the MethodInvoker interface define method
>    setParameter(int i, Object value)
> that the various ParamRule objects could then call, rather than directly
> accessing a "parameter stack" data structure.

+1 for local storage

> *
> having each CallMethodRule (which implements MethodInvoker) manage its own 
stack
> of parameter data. This could still be a stack of arrays internally, but it 
is
> now a data structure private to the class, which is better than the current
> paramstack object which is exposed everywhere.
> The bit about XXParamRule classes scanning the list of rules to find the
> previous CallMethodRule means that code like this:
>   digester.addCallMethod(....);  // rule 1
>   digester.addCallParam(...);   // automatically associates with rule 1
>   digester.addCallMethod(....);  // rule 2
>   digester.addCallParam(...);   // automatically associates with rule 2
> works in an intuitive fashion. A CallParamRule associates with the most 
recently
> added CallMethodRule.

But:
digester.addCallMethod(....);  // rule 1
digester.addCallParam(...);   // does not automatically associate with param 1 
(1 relative)
digester.addCallParam(...);   // does not automatically associate with param 2
digester.addCallParam(...);   // does not automatically associate with param 3
Catch my drift?

> None of this should need changes to the digester core as far as I can 
see;<snip>

Yes, didn't intend to cast any doubt on that.

> I haven't felt the enthusiasm to test out the idea, look for flaws, implement
> it, add unit tests, etc. for digester 1.x series.
> I certainly do intend to do *something* to address the existing 
CallMethodRule
> issue though for the digester 2.x series. [which has no release date planned 
yet].

I can propose a few iterations, if you wish, as and when I get some time.

(In reply to comment #3)
> One minor note: when attaching files, type=patch should only be used for text
> files that can be fed to the "patch" program, eg generated by "svn diff" 
or "cvs
> diff". Zip files containing patches should be type=binary. Thanks.

Let me know if you want me to re-attach.

-Rahul


-- 
Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.

---------------------------------------------------------------------
To unsubscribe, e-mail: commons-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: commons-dev-help@jakarta.apache.org


Mime
View raw message