Re: [Perl-workflow-devel] [rt.cpan.org #38024] all conditions with 'test' attirbute appears to be cached as 'evaluate'

[Jonas B. N. <jo...@gm...>]

Hi Jim,

Thanks for the explanation, it was something along the lines of what I  
expected. Not being a user of this functionality however, I just  
needed some hand-holding on the patch evaluation.

I will await your feedback from talking to Robert.

jonasbn

On 12/11/2008, at 21.37, Jim Brandt wrote:

> I'm ok with the principle of the patch. The special 'test'  
> conditions are conditions with raw Perl code rather than an explicit  
> condition object. The _create_condition_objects sub then creates a  
> condition object on the fly for each of these. The problem is the  
> name of each auto-generated condition object is hard-coded to  
> 'evaluate'.
>
> I would want to verify if the 'evaluate' name is a magic name used  
> somewhere else in the code such that it expects it to be hard coded  
> that way.
>
> Also, the patch uses the value of the test attribute as the name.  
> While this would guarantee that it will be unique, it could be an  
> arbitrarily large hash key (you can put in as much perl code as you  
> want) and it wouldn't be very human readable if you were ever  
> debugging.
>
> I would suggest adding some code to name it something like:
>
> "_" . <action_name> . "_condition_" . $counter
>
> where action_name is the name of the enclosing action and the  
> counter is to allow us to have multiple 'test' type conditions for  
> an action. The leading underscore is just to indicate that is is  
> private (auto-generated). So if you were debugging, the names of  
> these condition objects would look something like:
>
> _MyAction_condition_1
>
> I work with Bob (Robert), so we'll discuss in the next day or two.
>
> Jim
>
>
> Jonas Brømsø Nielsen wrote:
>> Hello All,
>> I have not received any feedback and I am reluctant to making a
>> development release for a one-line patch, I was hoping we would be
>> able to evaluate this and decide simply on the contents of the patch.
>> The issue is described below and the patch goes for line: 349 in HEAD
>> The line:
>>        name  => 'evaluate',
>> Roberts wants changed to:
>>        name => "_$condition_info->{test}",
>> I have not really gotten my head around the condition objects (I
>> should do this soon since our docs needs a serious update).
>> But this comment just above makes me hesitate applying the patch:
>>        # Special case: a 'test' denotes our 'evaluate' condition
>> So I guess the evaluate name is supposed to be hard-coded. However
>> Roberts patch does seem to offer some flexibility. But then the _
>> (underscore) troubles me.
>> I am going to go over the tests covering this part later tonight, but
>> if anybody has suggestions or opinions on this please feel free to
>> write me (the list). Jim, I am particularly interested in your
>> opinion :)
>> Now I will go for my run to clear my head then I will look at the  
>> code.
>> jonasbn
>> On 24/10/2008, at 23.55, Jonas Brømsø Nielsen wrote:
>>> Hiya Alll,
>>>
>>> We have received a patch from Robert Stockdale.
>>>
>>> If anybody has time to evaluate it, please feel free.
>>>
>>> jonasbn
>>>
>>> Begin forwarded message:
>>>
>>>> From: "Robert Stockdale via RT" <bug...@rt...>
>>>> Date: 29. sep 2008 21.48.42 GMT+02:00
>>>> To: undisclosed-recipients:;
>>>> Subject: [rt.cpan.org #38024] all conditions with 'test' attirbute
>>>> appears to be cached as 'evaluate'
>>>> Reply-To: bug...@rt...
>>>>
>>>>     Queue: Workflow
>>>> Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=38024 >
>>>>
>>>> Hi Jonas,
>>>>
>>>> Sorry for the delay. I was able to confirm this bug.
>>>>
>>>> The name is being explicitly set to 'evaluate' in
>>>> Workflow::State::_create_condition_objects for all conditions that
>>>> specify 'test' as an attribute in the xml configuration. This,
>>>> coupled
>>>> along with caching of condition results means that only the first
>>>> 'evaluate' condition will actually be evaluated and each subsequent
>>>> call
>>>> will use the cached result.
>>>>
>>>> Attached is a patch that will resolve the issue. Essentially I am
>>>> just
>>>> taking the value of the 'test' attribute and naming the condition
>>>> that,
>>>> with one small exception. Perl seems to have some issue with keys
>>>> that
>>>> start with '!' so I prefixed it with an underscore. I tested this
>>>> change
>>>> and it is working in my dev environment.
>>>>
>>>> An alternative option would be to allow the user to specify the
>>>> 'name'
>>>> attribute in the xml just like with any other condition. I did not
>>>> exercise this option.
>>>>
>>>> -Bob
>>> <evaluate_condition_name.patch>
>
> -- 
> Jim Brandt
> Administrative Computing Services
> University at Buffalo
>