nskins / goby

Command-line role-playing game framework
MIT License
122 stars 56 forks source link

93 abstract input #95

Closed rdavid1099 closed 7 years ago

rdavid1099 commented 7 years ago

player_input now versatile enough to be handled throughout the framework.

NOTES:

Tests added to util_spec to test lowercase functionality.

nskins commented 7 years ago

Requested changes:

Thank you!

rdavid1099 commented 7 years ago

Just a heads up, I had to change the test block in util.rb#player_input in order to test the new arguments.

    if(ENV['TEST'] == 'rspec')
      print prompt
      input = gets.chomp
      puts "\n" if doublespace
    else

Updating Ayara now.

rdavid1099 commented 7 years ago

@nskins Requested changes taken care of and ready for review :)

nskins commented 7 years ago

There's one more player_input in lib/driver.rb for which you should set the prompt. As for the test block of player_input that you mention above, can you think of some way to DRY the printing of the prompt and the newline? Right now those are repeated, but there's a simpler way to write it.

Everything else is looking good!

rdavid1099 commented 7 years ago

This is a bit controversial, so I wanted to check with you before committing and pushing.

    input = if(ENV['TEST'] == 'rspec')
              print prompt
              gets.chomp
            else
              Readline.readline(prompt, false)
            end
    puts "\n" if doublespace
nskins commented 7 years ago

The only problem is that prompt is in two spots (non-DRY). How about this?

print prompt
input = (ENV['TEST'] == 'rspec') ? gets.chomp : Readline.readline('', false)
puts "\n" if doublespace
rdavid1099 commented 7 years ago

Nice, @nskins. I like it. It's fixed and pushed.

nskins commented 7 years ago

Thank you! As you can see on the README, I am currently working on writing tests over the entire codebase. Once I've finished, I will have one more task for you, which is to merge your changes into the new codebase. I will let you know when that time comes.

rdavid1099 commented 7 years ago

You got it 👍

nskins commented 7 years ago

Hi Ryan, I'll handle the merge conflict myself since I don't want you to worry about all the code I wrote recently (there's a lot). Will let you know about something else you can work on soon.