Lorenzo Alberton [2004-03-13 00:37 UTC] Great job, Michael!
Just a few pedantic notes:
- FILE Gettext.php, LINE 98 - missing ";"
- idem for FILE MO.php, LINE 236.
- in File_Gettext_MO::_read(), add a check on $bytes, it must be >0.
- add some more verbose phpdoc for important methods.
Apart from that, I'm looking forward to see this pkg in pear. I already put it in my local repository :)
Michael Wallner [2004-03-13 09:45 UTC] Thanks for your comments, Lorenzo.
Most issues already were fixed while you wrote that message, though the package file should be completely broken - sorry for that, I'll fix ASAP.
Mike
Jan Schneider [2004-03-13 10:00 UTC] Very nice package! You shouldn't use $php_errormsg in the factory though, for portability reasons.
Michael Wallner [2004-03-13 10:08 UTC] Thanks, Jan.
I used $php_errormsg in the load() and save() methods and PEAR.php calls ini_set('track_errors', true), so though a problem?
Mike
Lorenzo Alberton [2004-03-13 10:33 UTC] I checked cvs version (1.4) and runs fine.
Just a few more pedantic notes:
- require_once is a statement, not a function, so the parenthesis are not needed:
require_once 'myfile.php';
- toArray() and fromArray() should not be needed in MO/PO.php since they're inherited by the extended class.
- in File_Gettext::fromArray(), the order of array_key_exists() parameters is switched
- in File_Gettext::poFile2moFile(), $mofile should be created if not exists yet, instead of raising an error:
if (!is_file($mofile)) {
touch($mofile);
}
Apart for that, I'm really happy with it!
Lorenzo
Michael Wallner [2004-03-13 10:42 UTC] Regarding Lorenzos comment:
- require_once: if it isn't a big problem I'd really like to keep that syntax - the CS guide only says I don't *need* parenthesis
- File_Gettext::fromArray(): fixed - sorry for that
- childs fromArray() and toArray(): I'll reconsider this anyways
- poFile2moFile(): indeed
Thanks,
Michael
Michael Wallner [2004-03-13 10:49 UTC] Updated PEAR package to current CVS.
Alan Knowles [2004-03-13 11:31 UTC] Seems good - only minor comment:
po2mo() + poFile2moFile() seem a little klunky. and load/save look like having a filename as an arg would be sensible.
eg.
$po = File_GetTexT::factory('MO');
$po->load('somefile')
$mo = $po->toMo();
$mo->save('filename');
or even.. (if you have different source formats...)
$fl = File_GetText::factory('FlexyReader');
$fl->load('mytemplate.html');
$mo = $fl->toMo();
$mo->save('mytemplate.mo');
Michael Wallner [2004-03-13 12:05 UTC] You're definitely right, Alan.
Changes are implemented and the package was updated.
Thanks
Jan Schneider [2004-03-13 12:09 UTC] Re $php_errormsg: I didn't know that PEAR.php sets track_errors. It's fine then.
|
