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

[Stockdale, R. <rf...@bu...>]

I attached the updated patch file to the RT ticket:

    http://rt.cpan.org/Ticket/Display.html?id=38024

Also, I wrote some automated tests for this change. What is the best way to
get them into the distribution?

--
Bob Stockdale
Administrative Computing Services
University at Buffalo
(716) 645-5334
rf...@bu...


> From: Jonas Brømsø Nielsen <jo...@gm...>
> Date: Mon, 17 Nov 2008 15:22:42 -0500
> To: "Brandt, Carl" <cb...@bu...>
> Cc: Workflow development list <per...@li...>
> Subject: Re: [Perl-workflow-devel] [rt.cpan.org #38024] all conditions with
> 'test' attirbute appears to be cached as 'evaluate'
>
> 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
>>
>
>
> -------------------------------------------------------------------------
> This SF.Net email is sponsored by the Moblin Your Move Developer's challenge
> Build the coolest Linux based applications with Moblin SDK & win great prizes
> Grand prize is a trip for two to an Open Source event anywhere in the world
> http://moblin-contest.org/redirect.php?banner_id=100&url=/
> _______________________________________________
> Perl-workflow-devel mailing list
> Per...@li...
> https://lists.sourceforge.net/lists/listinfo/perl-workflow-devel