Comments for "File_Gettext"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments
  • 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.

  • 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?

  • 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)) {

    Apart for that, I'm really happy with it!

  • 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

  • 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.

    $po = File_GetTexT::factory('MO');
    $mo = $po->toMo();

    or even.. (if you have different source formats...)
    $fl = File_GetText::factory('FlexyReader');
    $mo = $fl->toMo();
  • Michael Wallner  [2004-03-13 12:05 UTC]

    You're definitely right, Alan.

    Changes are implemented and the package was updated.

  • 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.