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

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

Jonas,

I already have a SourceForge account so that is probably the easiest
solution for everyone.

Could you please grant me commit rights? Also, which branch would you prefer
I work in?

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


> From: Jonas Brømsø Nielsen <jo...@gm...>
> Date: Tue, 18 Nov 2008 14:45:59 -0500
> To: Robert Stockdale <rf...@bu...>
> Cc: "Brandt, Carl" <cb...@bu...>, 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'
>
> Hi Robert,
>
> You are more than welcome to get commit rights to our hosted
> subversion repository.
>
> We are currently hosted at SourceForge so it requires an account there.
>
> Or you can send me the whole thing and I will sort it out.
>
> Thanks for taking the time and effort to do the extra iteration, I
> think it will prove to be worth while.
>
> jonasbn
>
> On 18/11/2008, at 20.37, Stockdale, Robert wrote:
>
>> 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
>>
>