Closed emcmillan closed 10 years ago
Commit looks good to me, except that it is really two commits merged into a single one. Any chance you could split them up? (hint: git add -p
, or git reset -p HEAD^
in this case are extremely useful tools, and doing a forced push to your branch automatically updates this PR)
I’ll try splitting them up, but I’m no Git expert and have no clue where to start. Truthfully, I made the edits and tested in my local code and then cut and pasted them into the online Git editor J
From: Matthijs Kooijman [mailto:notifications@github.com] Sent: Wednesday, June 25, 2014 1:14 AM To: Pinoccio/library-pinoccio Cc: emcmillan Subject: Re: [library-pinoccio] Allow multiple args to hq.Report. Fix key leak (#141)
Commit looks good to me, except that it is really two commits merged into a single one. Any chance you could split them up? (hint: git add -p, or git reset -p HEAD^ in this case are extremely useful tools, and doing a forced push to your branch automatically updates this PR)
— Reply to this email directly or view it on GitHub https://github.com/Pinoccio/library-pinoccio/pull/141#issuecomment-47072607 . https://github.com/notifications/beacon/7103446__eyJzY29wZSI6Ik5ld3NpZXM6QmVhY29uIiwiZXhwaXJlcyI6MTcxOTMwMzI2MiwiZGF0YSI6eyJpZCI6MzUyMDIwMTN9fQ==--c7bfc970c36faf8455308341cc346d22669fbd59.gif
I can recommend digging into git, it's really worth the investment since it's a brilliant tool (can't recommend any good tutorials, though...). Don't worry about this pullrequest though - when I get around to merging it, I'll take care of splitting it as well (though feel free to try yourself if you feel up to it before I get to merging).
One more remark though: If you reply to tickets through mail, better remove the original mail from below, to prevent cluttering your comment (look at the webinterface to see what I mean).
This is :thumbsup: from me too, merge whenever you'd like @matthijskooijman after any git tweeks :)
I'm not sure how to do the git magic either but I want to get this fix in today so I'm going to merge it :)
This allows multiple arguments to be passed to hq.Report(). The functionality was all there, but the checkArgs() call at the top of hqReport() was checking for exactly 2 arguments.
Also noticed a bug in arg2array. It was allocating a "sticky" key for each string argument but not freeing them. I changed the "at" flag from 0 to 1 to allow keyLoop to automatically free the keys on each Bitlash loop. hq.Report still reports the correct data but now it won't fill the key table.