|
|
|
I'm working on a small test for this. I'll post it as soon as I have it completed.
Bruce, any chance of posting the 'small' test(s) ?
I just looked into this patch and I don't think this is ready to be tested. It
seems that the patch is assuming two things will always be true: 1) All boolean accessors will use the is<field-name>() nomenclature The first assumption can be fixed by testing for both is<field-name() and The second assumption can be fixed by using Boolean.TYPE to represent the > The second assumption can be fixed by using Boolean.TYPE to represent the
> primitive Actually, in this case, just continue to use "boolean" to represent the primtive. Nick, Bruce, can you please point me to the place where this feature is
documented for the user ? I have just been browing http://castor.exolab.org for some time now, but couldn't find anything related to this. I'm assuming you are referring to the method signatures and such? If so the only
place I know if is #2 below this link http://castor.exolab.org/jdo-mapping. html#The-<field>-element . If this isn't what you are talking about could you be more specific? Bruce, also in comment to your first argument. Isn't there an assumption anyways
that the methods are going to be <get><whatever> format, so why cant we use the <is><whatever> for booleans? Every API doc I've seen uses this standard signature so I dont see that we are "assuming" anything more then we are with the <get> approach. > I'm assuming you are referring to the method signatures and such? If
> so the only place I know if is #2 below this link > http://castor.exolab.org/jdo-mapping.html#The-<field>-element. I have just been reading this page again, and I simply cannot find any reference (s) to such a feature, where the getter method for a boolean property will be set to is<CapitalizedName>(). Oh, well that's because its not there.
That's why I filed this bug, I cause it should be reclassified as an "enhancment" instead of a bug. Oops, sorry for the misunderstanding. All the time whilst following this
thread, I had been under the impression that you are asking for an existing implementation to be fixed or to be brought into coherence with what the docs say. Having said that, I think that the implementation should be flexible in a way public boolean isXXX() is not present, Castor should fall back onto public boolean getXXX(). No problem Werner, I should have filed it correctly to begin with.
I was just thinking the same thing too, and am surprised I didnt do it that way Adding another patch for handling both get/is setup. Tell me what you think.
(Of course its not final as you'll see the comments, but is it ok to head down this direction?) Created an attachment (id=653)
New patch to handle get/is for booleans Nick, just looking at your amended patch now. As per our code submission
guidelines, would you be in a position to provide me with a JUnit test case that successfully tests this new behaviour. I suppose I could do that, sorry for not posting one earlier. It will be a
little bit before i get to it, but I'll see what I can come up with. > Bruce, also in comment to your first argument. Isn't there an assumption anyways
> that the methods are going to be <get><whatever> format, so why cant we use the > <is><whatever> for booleans? I actually agree with you here, Nick. In fact, that's what I stated: 1) All boolean accessors will use the is<field-name>() nomenclature The first assumption can be fixed by testing for both is<field-name() and > Every API doc I've seen uses this standard signature so I dont see that we are My point was that you need to test for both isSomething() and getSomething(), Alright, bearing the submission of a JUnit test case, your votes please !
Created an attachment (id=660)
Update patch for tests and other issues Alright, take a look at the above patch. I integrated the test into the
TypeHandling tests as that seems like the most logical place to put it. I've also update all the SQL Scripts accordingly. One other issue I found was in the throwing exception part which is unrelated to And on a final note. When I was trying to run the tests I couldn't because the And one more comment, Bruce I figured thats what you were talking about and the
new patches include the test for both get/is. Sorry for the mis-understanding there, but I think its resolved now. I haven't had a chance to test the patch, but it looks good. As for the test
failure, until I dig into it further, I'm not sure why it's occurring because the method is there. > One other issue I found was in the throwing exception part which is
> unrelated to this bug, but for some reason a 'null' was being passed > and the javaClass.getName() method was always returning void, so I fixed > those too. Should I open up a new bug for this and post the changes there? No, let's include this here. As we are already touching FieldMolder, we might take the opportunity to clean it up a little bit .. . > And on a final note. When I was trying to run the tests I couldn't because
> the mapping for the TestNested is messed up and I kept getting a stack > trace envolving value2 get method. In the end I just commented that out > in the mapping file and everything but the nested test ran fine. Including > my new addition. Can you please have one more look at your patch. I've just checked out a fresh copy from CVS and run 'test castor.mysql', and I do not get any of those errors related to the value2 property. It could be that your patch 'disables' Castor's ability to deal with nested It actualla did .. I am just in the process of posting another patch
(FieldMolder only) that shows the difference, for completeness sake. Other than this, I am ready to commit. Created an attachment (id=674)
Final patch (FieldMolder only). Committed as discussed above.
Btw, in src/tests/jdo/TypeHandling, I had to add testBooleanIsMethod() to Please do not forget to close this bug once you've assessed the code's correctness. Btw, please think about what needs to be changed in the documentation at
src/docs to reflect the addition of this new feature. Quick question, what was the cause for the nested attribute stuff? You said my
patch affected that? If so how? Just wondering cause I'm not sure what would have done that. As you enclosed the old code to find a getter with a
if (method == null) { one had to set method = null at the bottom of while (true) { ..} loop. Actually, Nick, it seems that your test case actually fails. Can you please
investigate ? Yep, I can. Might be able to get to it today, no promises though
Btw, there's no need to rush, as all test cases still work but the one you've
amended .. whereas they didn't work before I made the final change. Created an attachment (id=681)
mapping file patch Found the problem. It looks like the mapping file simply wasnt patched. Doh! Oh
well, attached the patch file for it. Should work now! . I'll take care of this as time permits.
Reopened as per missing patch for mapping file.
About to check this (last !!!) patch in. If you fancy, have a look at
src/tests/jdo/TypeHandling.java again, and its runTest() method in particular. I tried to add testSimpleFloat() to it, but this produces an AssertionFailedError .. . Nick, I really appreciate your efforts, but what about providing me with a patch
for the documentation, so that this new feature is properly documented. Will do that as well werner. Should have it to you by this afternoon.
Created an attachment (id=690)
document patch Attached the document patch. It seems a little clumsy to me, but not sure if
anything else is possible for this. Its a start and will work for now I think. Created an attachment (id=691)
edited doc patch Well just though of another way to word it. Pick which one sounds best to you
Committed. Please do not forget to close this bug report once you've assessed
its correctness. |
Patch file to replace get/is depending on type