Re: termbox-php

Thanks for the review Johannes. I just pushed the following changes: * Include guard + license in constants.h * Replaced rogue tabs with spaces * Replace reference params with mixed return types * Use zend_parse_parameters_none for zero-arg functions * Use add_assoc_long_ex for constant string keys I'll request an account. Adam On Fri, Sep 5, 2014 at 8:52 PM, Johannes Schlüter <johannes@php.net> wrote: > Hi, > > On Sat, 2014-08-23 at 03:59 -0400, adam@atoi.cc wrote: >> I've written a wrapper extension for termbox[1] that I think would >> make a nice addition to PECL. >> >> https://github.com/adsr/termbox-php >> >> Feedback appreciated! > > As said on IRC some comments from > http://news.php.net/php.pecl.dev/12223 > apply, too. Additionally constants.h should have a license header and > include guard and you seem to mix space and tabs for indention. > > On an API design note I'd suggest that termbox_peek_event > termbox_poll_event don't return by reference but maybe an array like > [ "type" => TB_EVENT_*, "event" => [ ... ] ] on success and error > code > or such on failure. Returning by reference is C-style and uncommon in > PHP. Also I think your way of returning by ref is wrong and will fail in > cases like $a = null; $b = $a; termbox_poll_event($a); as $b will be > changed, too, while it shouldn't. For receiving references you have to > tell the engine to separate before calling the function using ARGINFO. > In > > http://schlueters.de/blog/archives/125-Do-not-use-PHP-references.html > > http://schlueters.de/blog/archives/141-References-and-foreach.html > and https://www.youtube.com/watch?v=bxxIXPc9IR8 > I provide some reasons for not using references ... and frankly would > have to check myself on how to implement that properly. > > Aside from that it looks fine. > > If nobody else complains feel free to request PECL account for > publishing on pecl.php.net via > http://pecl.php.net/account-request.php . > If you want a php.net account to host the primary git repo on > git.php.net (will be mirrored to github) and bug tracker make sure to > set the checkbox (unless you already have an account) and tell us so we > can enable that. > > johannes