Comments for "File_CSV_Get"

» Submit Your Comment
Comments are only accepted during the "Proposal" phase. This proposal is currently in the "Finished" phase.
» Comments

  • Till Klampaeckel  [2008-11-15 12:55 UTC]

    Very sweet idea, this package is like a dream come true. :-)

    I checked out your code and looked at it very briefly, ran tests - great documentation, great examples.

    There are small things I'd like you to change, for example, using protected instead of private so people can extend/overload your class and still access the properties and methods. Or making the code more robust by checking that $res is not "false" in File_CSV_Get::_parse(), etc..

    If you keep as-is, think about file_exists() vs. is_readable(). I *think* on some systems there can be a difference, which can lead to a nasty error. ;-)

    I'd also like to be able to "open" a URL which outputs CSV-data, and then I wouldn't force the filename/URL to end in ".csv". In general, some operating systems tend to omitt file extensions anyway, so in that case I'd assume the file/URL contains CSV-data and make File_CSV_Get::_validate() more robust and handle an error.

    If you really want to stick to ".csv", I'd at least replace the check with "substr($file, -4)".

    Aside from those the package.xml, tests and maybe the structure in your SVN needs some minor fixes, I can help you if you want. Those are just minors though. ;-)

    All in all, great idea and really well done!

  • Kazuyoshi Tlacaelel  [2008-11-17 11:23 UTC]

    Till, Thanks for you comments.
    You have a great attitude, cheers!


    > I checked out your code and looked at it very briefly, ran tests - great
    > documentation, great examples.

    Thanks, there is more to do, but right now I though of making the proposal
    and make some changes to the code before proceeding with documentation ; )

    > There are small things I'd like you to change, for example, using
    > protected instead of private so people can extend/overload your class

    I agree with you, good observation!

    In the beginning I though of making them private so people don't start using something that may change.

    I'll keep the patch methods private, and change some methods visibility!

    > If you keep as-is, think about file_exists() vs. is_readable(). I *think*
    > on some systems there can be a difference, which can lead to a nasty error.
    > ;-)

    Yeap, sounds good, I'll apply this changes too.
    I also like the idea of the url, I will start thinking on that as well.
    yes! the file extension is supposed to go!

    I added some you proposals to the issues on google-code
    and will come back to this post to see if I missed something.
    http://code.google.com/p/php-csv-parser/issues/list

    About the svn file structure..
    What should I change?
    I had my own doubts about that, about
    - "tests/data" going to "data" instead
    - and misc.

    I normally just check out trunk and rename it to "CSV"
    under a "File" directory that is not under subversion.

    then under "CSV" run

    $ sudo ./build

    This unistalls the package.
    runs "phpcs", "phpdoc", "phpunit" adds phpdocumentor compiled docs
    to package.xml and builds the package, terminating by re-installing it,
    and logging the results.

    Is there any task that runs "all pear-projects test-code" ?
    should I change my test structure to match that?

    Cheers!