Bug #27570 for Data-FormValidator: DFV::ConstraintFactory broken (and untested!)

Wed Jun 13 18:50:14 2007 GTERMARS [...] cpan.org - Ticket created

Subject:

DFV::ConstraintFactory broken (and untested!)

I recently wanted to make use of some of the methods in DFV::ConstraintFactory, but couldn't get any of them to work properly. Dug a bit deeper into the code and saw that this module (1) doesn't have a test suite, and (2) doesn't work properly. Doh! I've attached a patch here which addresses both issues... Firstly, it updates the "t/constraint_factory.t" test suite so that -all- of the methods in this module now have a test suite. Secondly, it fixes the actual DFV::ConstraintFactory module so that it actually now passes the tests. FYI, although I agree that some of the function names are unwieldy, I also think that its useful to have these constraints around; its a whole lot quicker/easier to just use "make_set_constraint()" or "make_clamp_constraint()" than it is to build your own constraint methods.

Subject:

dfv-constraint-factory.patch

Message body is not shown because it is too large.

Thu Jun 14 11:53:23 2007 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #27570] DFV::ConstraintFactory broken (and untested!)
Date:	Thu, 14 Jun 2007 10:01:59 -0400
To:	bug-Data-FormValidator [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

> > I recently wanted to make use of some of the methods in > DFV::ConstraintFactory, but couldn't get any of them to work properly. > Dug a bit deeper into the code and saw that this module (1) doesn't > have a test suite, and (2) doesn't work properly. Doh!

Great. Thanks for your help on this. These came with the project when I took over maintenance, and I never had the energy to work on them, since I haven't really used them. I expect to apply your patches, pending a final review. Thanks again, Mark -- . . . . 1997-2007: Ten Years of Excellence. . . . . . Mark Stosberg Principal Developer mark@summersault.com Summersault, LLC 765-939-9301 ext 202 database driven websites . . . . . http://www.summersault.com/ . . . . . . . .

Thu Jun 14 11:53:25 2007 The RT System itself - Status changed from 'new' to 'open'

Thu Jun 14 12:48:16 2007 GTERMARS [...] cpan.org - Correspondence added

On Thu Jun 14 11:53:23 2007, mark@summersault.com wrote: Show quoted text

> Great. Thanks for your help on this. These came with the project > when I took over maintenance, and I never had the energy to work > on them, since I haven't really used them.

You're welcome. I was actually a bit surprised to see that they weren't working, and figured that it was either (a) because they'd been deprecated, or (b) that there just were enough round 'tuits (BTW, I've got lots of square ones if anyone wants to trade). In my current project I actually had need to make use of several of these methods for building up a chain of required/optional constraints, and when I saw they weren't working I figured that I could either roll my own custom ones for the project or just fix the ones in DFV. Gee, wasn't hard to see which path I took. :) Show quoted text

> I expect to apply your patches, pending a final review.

Appreciated. Looking forward to a new DFV release! <hint, hint> ;)

Fri Jul 13 23:13:59 2007 MARKSTOS [...] cpan.org - Taken

Fri Jul 13 23:27:51 2007 MARKSTOS [...] cpan.org - Correspondence added

On Wed Jun 13 18:50:14 2007, GTERMARS wrote: Show quoted text

> I recently wanted to make use of some of the methods in > DFV::ConstraintFactory, but couldn't get any of them to work properly. > Dug a bit deeper into the code and saw that this module (1) doesn't > have a test suite, and (2) doesn't work properly. Doh! > > I've attached a patch here which addresses both issues... > > Firstly, it updates the "t/constraint_factory.t" test suite so > that -all- of the methods in this module now have a test suite. > > Secondly, it fixes the actual DFV::ConstraintFactory module so that it > actually now passes the tests. > > FYI, although I agree that some of the function names are unwieldy, I > also think that its useful to have these constraints around; its a > whole lot quicker/easier to just use "make_set_constraint()" > or "make_clamp_constraint()" than it is to build your own constraint > methods.

I've reviewed this patch now. There are two kinds of constraints in DFV. There is the old kind, which received the value as the input, and there are the new "constraint methods", which pass in a DFV object as the first argument. Your patch didn't "fix" the constraint factory so much as it changed the behavior of the constraints generated from the old way to the new way. Applying your patch will surely break someones code if they upgrade DFV, perhaps even some of my own code. In the ::Constraints module, I made all the constraints available the old and a new ways, through some hoop jumping. The reality is that the new way you've switched it to is better, and supporting both is more complicated, without adding much value, except backwards compatibility. However, DFV has generally always tried to be backwards compatible, and I want to continue that trend. I suggest releasing the updated module and tests in their own distribution, under the name Data::FormValidator::ConstraintMethodsFactory. And since it won't need to be compatible then, improve some of the names and add and remove usual functions as you see fit! For example, maybe "make_or_constraint" could become just "FV_or". (The FV_ prefix is used some other new DFV constraint methods). I would be happy to then update the DFV docs to promote the new solution , and would likely start using it myself. Mark

Mon Jul 30 17:19:57 2007 GTERMARS [...] cpan.org - Correspondence added

From:

GTERMARS [...] cpan.org

On Fri Jul 13 23:27:51 2007, MARKSTOS wrote: Show quoted text

> On Wed Jun 13 18:50:14 2007, GTERMARS wrote: > I've reviewed this patch now. > > There are two kinds of constraints in DFV. There is the old kind, > which received the value as the input, and there are the new > "constraint methods", which pass in a DFV object as the first > argument. > > Your patch didn't "fix" the constraint factory so much as it > changed the behavior of the constraints generated from the old > way to the new way. > > Applying your patch will surely break someones code if they upgrade > DFV, perhaps even some of my own code.

Appreciate your comments Mark! And you're right... I hadn't really put much thought into preserving the "old style" constraints and how they were handled; I've been using the "new style" ones and find them -MUCH- more useful/powerful than the other ones. I've gone ahead and have spun this material out into a new "Data::FormValidator::Constraints::MethodsFactory" package, which I'll be uploading to CPAN/PAUSE shortly.

Tue Jul 31 09:59:01 2007 mark [...] summersault.com - Correspondence added

Subject:	Re: [rt.cpan.org #27570] DFV::ConstraintFactory broken (and untested!)
Date:	Tue, 31 Jul 2007 09:58:07 -0400
To:	bug-Data-FormValidator [...] rt.cpan.org
From:	Mark Stosberg <mark [...] summersault.com>

Show quoted text

> > I've gone ahead and have spun this material out into a > new "Data::FormValidator::Constraints::MethodsFactory" package, which > I'll be uploading to CPAN/PAUSE shortly.

I took a look at it last night. Looks great! I like the improved names. I'll be more likely to use this myself. Mark

Sat Oct 20 15:17:07 2007 MARKSTOS [...] cpan.org - Correspondence added

I have now updated the docs in Data::FormValidator to reference the new module. I'm stalling this ticket until the module release happens.

Sat Oct 20 15:17:11 2007 MARKSTOS [...] cpan.org - Status changed from 'open' to 'stalled'

Sat Oct 20 16:03:36 2007 MARKSTOS [...] cpan.org - Correspondence added

4.53 has been sent to CPAN to address this.

Sat Oct 20 16:03:45 2007 The RT System itself - Status changed from 'stalled' to 'open'

Sat Oct 20 16:03:50 2007 MARKSTOS [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #27570 for Data-FormValidator: DFV::ConstraintFactory broken (and untested!)

Maintainer(s)' notes