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

[Jim B. <cb...@bu...>]

We discussed and came up with the following. Bob will be following up 
with a patch after some testing.

* With regard to conditions, the name 'evaluate' is only used when 
creating the Condition::Evaluate object. It seems when conditions are 
evaluated, the sub just cycles through all conditions in the conditions 
array, so the name 'evaluate' isn't special in any way.

* Naming for the auto-generated conditions

When the condition fails, the name of the condition is written out to 
log4perl, so previous to the patch users would see that the 'evaluate' 
condition failed, which tells them that it was a 'test' condition, but 
does not tell them which one.

Bob suggested even more detail in the name, like:

"_$state\_$action\_condition\_$counter" so for example:

_Begin_SA_condition_1

If you had a problem, this would point you directly to the 
state->action->in-line condition where your perl code is. I believe Bob 
said all of that information is easy to get inside the state object.


Jim

Jonas Brømsø Nielsen wrote:
> 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
>>
> 

-- 
Jim Brandt
Administrative Computing Services
University at Buffalo