Re: [PEPr] +1 for Web Services::Services_Amazon_SQS

On Mon, 2008-12-08 at 18:42 +0000, Till Klampaeckel wrote: > Till Klampaeckel (http://pear.php.net/user/till) has voted +1 on the proposal for Web > Services::Services_Amazon_SQS. > > Proposal information: > http://pear.php.net/pepr/pepr-proposal-show.php?id=582 > Vote information: > > http://pear.php.net/pepr/pepr-vote-show.php?id=582&handle=till > > This vote is conditional. The condition is: > > Code is clean and well document, a few suggestions though: > > 1) The shebang in scripts/sqs should be "/usr/bin/env php" or a replacement > task @php_bin@ (vs. hardcoded /usr/bin/php). For example, on most of the > better OS' (e.g. Unix :-P) the php-cli is in /usr/local/bin. ;-)) Will do. I'll use phpcs as an example. > 2) I also propose phpsqs instead of sqs - maybe through "addInstallAs()". I'm not sure about this one. I asked in IRC about this and opinions were mixed. The majority seemed to think including 'php' was unnecessary if there was no existing command with the same name. > 3) Also, maybe add a .bat for windows? (phpsqs.bat) Or is there any > dependency which would not allow Windows users to run your code. There are no dependencies that would prevent the package (or command-line script) from running in Windows. I plan to add a .bat file for the first release. Again, I'll be using phpcs as a model for how this is done. > 4) File structure in SVN should be ./pkgdir/Services/Amazon/ vs. > ./pkgdir/Amazon I think most packages use the convention I've used, which excludes the category directory in VCS. > 5) A general question/concern -- there's no Services_Amazon_SQS "hub" class, > right? Somewhat of a concern, but not a major one. If one class needs to fill this role, it would be Services_Amazon_SQS_Client. > I know that your "sqs" script basically implements all your functions, but > if I wanted to use your code straight (from another class), I'd be in > favour of using your classes vs. embedding exec() or system() in my own code. > > Anyway, looking briefly at your code, I think I would need to copy > Services_Amazon_SQS_Application into my own library and work based on that. > (So it seems.) So instead of that, I'd propose you move > Services_Amazon_SQS_Application from "scripts/sqs" to Services/Amazon/SQS.php > and include it in "sqs" instead. > > Reasons are a of course cleaner "sqs" app, and people can also easily re-use > your code for their own purposes. The Services_Amazon_SQS_Application class is only a thin command-line wrapper around the Services_Amazon_SQS_QueueManager class. Unless you want to extend the command-line application it doesn't make as much sense to include it with the other library classes. I'll obviously need to document the difference (in the package description and peardoc) to prevent confusion. > 5 is sort of a RFC -- not conditional. I just thought I'd include it. Thanks for the vote and your review comments, Mike > Cheers, > Till > > -- > Sent by PEPr, the automatic proposal system at > http://pear.php.net >