Re: svn commit: r826322 - /ofbiz/trunk/framework/minilang/src/org/ofbiz/minilang/method/entityops/StoreValue.java

[Adam Heath-2]

[hidden email] wrote:
> Author: lektran
> Date: Sun Oct 18 00:34:29 2009
> New Revision: 826322
>
> URL: http://svn.apache.org/viewvc?rev=826322&view=rev
> Log:
> Catch ClassCastExceptions in <store-value/> operations, these will occur if you have a null GenericValue (perhaps from a lookup that returned no results), set some fields on it and then try to store it.  What you actually have at that point is a FastMap created when you set the fields.
> Also added use of the utility MethodContext.setErrorReturn(String, SimpleMethod) rather than manually adding errors to the environment context.

Bad commit, this is 2 separate changes, should have been done as 2
separate commits.  One is a bug fix, the other is just a feature.

[Tim Ruppert]

Maybe you could leave out the "bad commit" part and just recommend the  
two separate commits :) - I like to see both features go in and bugs  
get fixed, so not so much bad as maybe another, even better way of  
handling it Scott!

Cheers,
Ruppert
--
Tim Ruppert
HotWax Media
http://www.hotwaxmedia.com

o:801.649.6594
f:801.649.6595

On Oct 20, 2009, at 11:29 AM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: lektran
>> Date: Sun Oct 18 00:34:29 2009
>> New Revision: 826322
>>
>> URL: http://svn.apache.org/viewvc?rev=826322&view=rev
>> Log:
>> Catch ClassCastExceptions in <store-value/> operations, these will  
>> occur if you have a null GenericValue (perhaps from a lookup that  
>> returned no results), set some fields on it and then try to store  
>> it.  What you actually have at that point is a FastMap created when  
>> you set the fields.
>> Also added use of the utility MethodContext.setErrorReturn(String,  
>> SimpleMethod) rather than manually adding errors to the environment  
>> context.
>
> Bad commit, this is 2 separate changes, should have been done as 2
> separate commits.  One is a bug fix, the other is just a feature.
>

[Adam Heath-2]

Tim Ruppert wrote:
> Maybe you could leave out the "bad commit" part and just recommend the
> two separate commits :) - I like to see both features go in and bugs get
> fixed, so not so much bad as maybe another, even better way of handling
> it Scott!

Yeah, should have said bad commit message.

[Scott Gray-2]

Aren't we getting a bit carried away with this?

The first was not a bug fix, catching the exception still results in  
an error, it's just that now you can actually understand what went  
wrong by looking at the logs.

As part of that improvement I wanted to return an error message and  
noticed that to do so required another 7 lines of code.  Must be an  
easier way I thought, that's when I came across the utility method in  
MethodContext which did exactly what I needed.  I chose to use that  
method for my change and also clean up the class by using it in the  
other two places that errors were being returned.

I've understood your reasoning in the past but if you expect me to  
split up super minor commits into tiny little digestible micro pieces,  
then no I won't, I'll just leave the code cleanups for someone who has  
the time to build, test and commit every 30 seconds.

Regards
Scott

HotWax Media
http://www.hotwaxmedia.com

On 21/10/2009, at 6:29 AM, Adam Heath wrote:

> [hidden email] wrote:
>> Author: lektran
>> Date: Sun Oct 18 00:34:29 2009
>> New Revision: 826322
>>
>> URL: http://svn.apache.org/viewvc?rev=826322&view=rev
>> Log:
>> Catch ClassCastExceptions in <store-value/> operations, these will  
>> occur if you have a null GenericValue (perhaps from a lookup that  
>> returned no results), set some fields on it and then try to store  
>> it.  What you actually have at that point is a FastMap created when  
>> you set the fields.
>> Also added use of the utility MethodContext.setErrorReturn(String,  
>> SimpleMethod) rather than manually adding errors to the environment  
>> context.
>
> Bad commit, this is 2 separate changes, should have been done as 2
> separate commits.  One is a bug fix, the other is just a feature.
>