Closed eric-hu closed 11 years ago
Thanks for the pull. ruby2 support is a higher priority. I need to offload a todo list of what's needed there before looking at this. In the meantime, I welcome @astashov, @deivid-rodriguez or anyone else to code review
I've had a quick look into this. The option parser looks good. As per the other test, I'm not sure whether spawning a new shell process in a new thread is the best way to go... Maybe reducing the executable to parsing the options with that RdebugOptionParse
class and calling the run
method of another RdebugRunner
class, and then just testing those two classes would be better? I'm just thinking loud...
@deivid-rodriguez thanks for the code review. I wasn't happy with the tests either, but wanted something that could run against the original code as-is to verify behavior as I changed things. I appreciate the suggestions on the tests and I'll work towards that direction.
No problem @eric-hu, I thinks this is a very nice enhancement and I'm looking forward to merging this into byebug
as well.
@cldwalker this code can be further refactored, but I wanted to run the changes by you before it diverged from cldwalker/master further. Could you look it over and let me know what you think?
Overview:
rdebug_test_slow.rb
is a test for rdebug that will drop-in to your master branch (along with the two supporting filessimple_class.rb
andsimple_loop.rb
). To be compatible with rdebug unmodified, it calls the rdebug executable with backticks and spawn. Unfortunately, that means that the client/server test has to loop on creating the client connection until the server has started up.rdebug_option_parser_test.rb
is only for the extracted class RdebugOptionParser in eric-hu/master. There's only a few tests in there right now, but others can be made in the same vein fairly easily, so long as this approach is acceptable.