On Mon, Jun 2, 2008 at 6:03 PM, Kevin Ballard <[EMAIL PROTECTED]> wrote: > Any comments/suggestions?
First off, great work. This is a major improvement to the libraries. Here's my notes from reviewing the commits (through 713371df7f): For those playing along, you can view these by going to http://github.com/kballard/thrift/commits/COMMIT 7a1b95c8d886 returns (lines 50, 31): The first item in the array (field name) isn't actually used/supplied currently May want to do the same in the tests for consistency. * I'm not really sure why it's returned at all. Legacy? c7101e6334f3b Consequences of including Thrift in the example group? 6cda1b66c24 On testing the remapped methods: Could we mock to expect the old method called by the new? Alternatively, if they're the same method (I don't think they are), it's possible method() could be used for equality. 665d95c51f What's going on here? 1d897acba89 Range errors on Bignum are ok but on integer aren't? 800706783f1 I think this might have been correct. We don't want the server bailing out if something goes wrong. We should discuss. 210803a358 I think switching sets to use Set breaks backwards compatability. There's also a bug we need to be aware of that allows arrays to be passed as sets (because .each doesn't care, and the value of the hash) isn't used. I think breaking backwards compat might be ok here, but we need to discuss on the list. 384064468d Ah, you got it (the set thing). That might be ok then. May want to do further testing. c83c437d7b38 I'm trying to remember if get_buffer is for the fast binary protocol. I think at this point it only uses borrow and consume. I'll double check. 5f29875a24 This is a good change. I think we should prefer testing with literals over method calls. e27daba8897 Wait what? A hash as the second arg to raise? -- Kevin Clark http://glu.ttono.us
