Commons Digester
  1. Commons Digester
  2. DIGESTER-37

[digester] Add support for CallMethodRule with target offset in xml rules

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: Windows XP
      Platform: PC

      Description

      It is currently not possible to specify a target offset on a CallMethodRule when
      using xml-based rules. The attribute needs to be added to the DTD, and the
      CallMethodRuleFactory class in DigesterRuleParser needs to be modified.

        Activity

        Hide
        Wendy Smoak added a comment -

        Not at all; I'm just glad to see it fixed and closed.

        BTW, earlier you asked:
        > Is this thing about having a string containing a comma-separated
        > list of types an established xmlrules convention,

        and I wanted to note that not only does Digester/xmlrules use comma-separated
        lists, but it also shows up in Ant:
        <target name="dist" depends="compile,javadoc" ...

        Show
        Wendy Smoak added a comment - Not at all; I'm just glad to see it fixed and closed. BTW, earlier you asked: > Is this thing about having a string containing a comma-separated > list of types an established xmlrules convention, and I wanted to note that not only does Digester/xmlrules use comma-separated lists, but it also shows up in Ant: <target name="dist" depends="compile,javadoc" ...
        Hide
        Simon Kitching added a comment -

        Thanks very much Wendy. Your code has been committed, with some minor changes.
        The (simple) changes make the test more obvious, though sadly a little less
        interesting . I hope you don't mind.

        Show
        Simon Kitching added a comment - Thanks very much Wendy. Your code has been committed, with some minor changes. The (simple) changes make the test more obvious, though sadly a little less interesting . I hope you don't mind.
        Hide
        Wendy Smoak added a comment -

        Created an attachment (id=14415)
        unit test for xmlrules use of call-method-rule targetoffset attrib

        Show
        Wendy Smoak added a comment - Created an attachment (id=14415) unit test for xmlrules use of call-method-rule targetoffset attrib
        Hide
        Simon Kitching added a comment -

        (In reply to comment #8)
        >
        > I have an idea of how write the test, but I don't know where to put it. Should
        > I write a 'testCallMethodRule' method in DigesterLoaderRulesTest?

        Yes, that seems like the best place to me.

        Show
        Simon Kitching added a comment - (In reply to comment #8) > > I have an idea of how write the test, but I don't know where to put it. Should > I write a 'testCallMethodRule' method in DigesterLoaderRulesTest? Yes, that seems like the best place to me.
        Hide
        Wendy Smoak added a comment -

        (In reply to comment #7)
        > Have you had a chance to test the new Digester code yet? Or even better to
        > create some unit tests? :=)

        I was looking at it last night. I thought I would just be adding another test
        to make sure the new targetoffset attribute was working properly, but I don't
        really see where CallMethodRule-from-xml-rules is being tested.

        There's a 'testObjectParamRule' method in DigesterLoaderRulesTest, that despite
        the name does use a <call-method-rule>. Have I missed it somewhere else?

        I have an idea of how write the test, but I don't know where to put it. Should
        I write a 'testCallMethodRule' method in DigesterLoaderRulesTest?

        Show
        Wendy Smoak added a comment - (In reply to comment #7) > Have you had a chance to test the new Digester code yet? Or even better to > create some unit tests? :=) I was looking at it last night. I thought I would just be adding another test to make sure the new targetoffset attribute was working properly, but I don't really see where CallMethodRule-from-xml-rules is being tested. There's a 'testObjectParamRule' method in DigesterLoaderRulesTest, that despite the name does use a <call-method-rule>. Have I missed it somewhere else? I have an idea of how write the test, but I don't know where to put it. Should I write a 'testCallMethodRule' method in DigesterLoaderRulesTest?
        Hide
        Simon Kitching added a comment -

        Hi Wendy,

        Have you had a chance to test the new Digester code yet? Or even better to
        create some unit tests? :=)

        Show
        Simon Kitching added a comment - Hi Wendy, Have you had a chance to test the new Digester code yet? Or even better to create some unit tests? :=)
        Hide
        Simon Kitching added a comment -

        I saw that the new function is called only once, but it still looks nice to
        separate it out; I am happy to leave it there. And performance-wise, I expect
        that a private method with only one caller would get "inlined" by the JVM anyway.

        Show
        Simon Kitching added a comment - I saw that the new function is called only once, but it still looks nice to separate it out; I am happy to leave it there. And performance-wise, I expect that a private method with only one caller would get "inlined" by the JVM anyway.
        Hide
        Wendy Smoak added a comment -

        (In reply to comment #4)
        > Ok, I've committed the patch to subversion, with some changes.

        I had a feeling I was making it more difficult than it needed to be. And now
        that private method is unnecessary; it only gets called once.

        So, what's the easiest way to clean this up? Is it easier to roll back this
        patch and let me do it over [with unit tests], or just keep going forward and
        patch this to move the contents of that method back inside?

        Whatever you think is best and/or less work for you is fine.

        Show
        Wendy Smoak added a comment - (In reply to comment #4) > Ok, I've committed the patch to subversion, with some changes. I had a feeling I was making it more difficult than it needed to be. And now that private method is unnecessary; it only gets called once. So, what's the easiest way to clean this up? Is it easier to roll back this patch and let me do it over [with unit tests] , or just keep going forward and patch this to move the contents of that method back inside? Whatever you think is best and/or less work for you is fine.
        Hide
        Simon Kitching added a comment -

        Ok, I've committed the patch to subversion, with some changes.

        The following are equivalent:
        new CallMethodRule(methodName);
        and
        new CallMethodRule(0, methodName); // targetoffset=0
        This fact allowed the code to be simplified while giving the same effect (I think).

        Wendy, would you mind testing this? It would be even better if you could provide
        a patch to the unit tests...

        And thanks for the patch..

        Show
        Simon Kitching added a comment - Ok, I've committed the patch to subversion, with some changes. The following are equivalent: new CallMethodRule(methodName); and new CallMethodRule(0, methodName); // targetoffset=0 This fact allowed the code to be simplified while giving the same effect (I think). Wendy, would you mind testing this? It would be even better if you could provide a patch to the unit tests... And thanks for the patch..
        Hide
        Wendy Smoak added a comment -

        Simon, if you're talking about this:
        StringTokenizer tokens = new StringTokenizer(paramTypesAttr, " \t\n\r,");
        I didn't add it, I just moved it out to a private method to avoid duplicating
        the code.

        Show
        Wendy Smoak added a comment - Simon, if you're talking about this: StringTokenizer tokens = new StringTokenizer(paramTypesAttr, " \t\n\r,"); I didn't add it, I just moved it out to a private method to avoid duplicating the code.
        Hide
        Simon Kitching added a comment -

        Hi Wendy,

        Is this thing about having a string containing a comma-separated list of types
        an established xmlrules convention, or is this patch the first time this has
        been done?

        Show
        Simon Kitching added a comment - Hi Wendy, Is this thing about having a string containing a comma-separated list of types an established xmlrules convention, or is this patch the first time this has been done?
        Hide
        Wendy Smoak added a comment -

        Created an attachment (id=14275)
        patch to DigesterRuleParser and DTD

        Show
        Wendy Smoak added a comment - Created an attachment (id=14275) patch to DigesterRuleParser and DTD

          People

          • Assignee:
            Unassigned
            Reporter:
            Wendy Smoak
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development