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

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

Sounds good.

I was started to thinking along the lines of using Data::UUID to  
generate the unique names, but that would probably just warp the  
developers minds completely.

I am looking for to the patch - we can get a development release out  
of the door the moment it can be applied. This will also let other  
developers evaluate the patch easily.


jonasbn

On 17/11/2008, at 21.18, Jim Brandt wrote:

> 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
>