Philippe Jausions [2007-01-19 16:02 UTC] The list of groups should go into a separate data file that can be updated without modifying the source code of the package...
Martin Jansen [2007-05-06 09:17 UTC] Can you put .phps files online somewhere? This way it is easier to browse through your soure code. Thanks!
Martin Jansen [2007-05-06 09:18 UTC] I think we need to find another name for this package. Usually we have the <prefix>_<name> schema for package names, which isn't the case here with name "ISBN". Text_ISBN perhaps?
Philippe Jausions [2007-05-15 14:20 UTC] Check the start of doc block comments, they don't look like they're indented properly, might be a tab instead of spaces.
Daniel O'Connor [2007-06-17 05:35 UTC] I'd be quite eager to see something like this in PEAR. A very cursory look at the API examples and source... no huge glaring problems, looks to be quite useful.
Ken Guest [2007-06-22 00:02 UTC] I'd be inclined to remove the validation of ISBN sequences in favour of using Validate_ISPN - which already validates ISBNs; and/or enhancing that validation class as requirements dictate.
It might also make sense to use one package to help validate/test the performance/reliability of the other.
No point in replication of effort - at the very least I'd suggest consulting the Validate_ISPN developers for their opinions.
Michael Gauthier [2009-07-03 04:15 UTC] This proposal looks to have some nice code in it. Are you going to finish it up?
Some quick suggestions if you do:
1.) use class constants instead of global defines
2.) put exceptions in a separate file
3.) variable names should use camelCase, not under_scores.
4.) give it a run through phpcs. It will pick up many minor problems with spacing and doc formatting.
5.) if you're passing parameters by reference, you'd better document it well!
6.) private static methods will be hard to write tests for. You may want to ask on the PHPUnit mailing list for guidance here.
7.) Regarding Ken's comment, the existence of Validate_ISPN is not a reason to discontinue development of this package. If anything, Validate_ISPN could just depend on this package if it gets accepted. This proposal seems to cover more aspects of ISBN than just validating.
Ken Guest [2009-07-03 11:41 UTC] I'm actually going to agree with Michael on this one; especially regarding point #7.
Michael Gauthier [2011-01-30 22:13 UTC] Tom, are you still working on this PEAR proposal?
|
