nskins / goby

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

Abstract battle() method #122

Closed scosbet closed 6 years ago

scosbet commented 6 years ago

Addresses #42

There is still a little more to do on this before it's completely finished but as I'm going away for a couple of weeks I wanted to submit a PR for review now so my approach thus far could be assessed.

I think the only important things left to do are to add some Player V Player tests, and to figure out how best to handle a Fighter Entity attempting to start a battle with a non-Fighter Entity.

As you can see there are a lot of changes so no rush in digesting it! Please feel free to comment/ask questions, I'll answer as best I can while away but will be much more attentive when I return.

Thanks!

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 24cffdbfa1e24cacb4e1b30d3d66539cfe0d0239 on scosbet:abstract_battle into 5935a64316f1300918ae484d3f4477a5f9a251b5 on nskins:master.

nskins commented 6 years ago

Awesome! Yes, it looks like you're heading in the right direction with this. I will probably look at this in more detail, but I'm a little tied up lately. If I haven't yet reviewed, feel free to leave a comment when you return and hopefully around then I will have a bit more time to take a second look. Thanks again!

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 24cffdbfa1e24cacb4e1b30d3d66539cfe0d0239 on scosbet:abstract_battle into 5935a64316f1300918ae484d3f4477a5f9a251b5 on nskins:master.

coveralls commented 6 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 02cfd79d5d6280e0b1e65816d460a29ca190852d on scosbet:abstract_battle into 5935a64316f1300918ae484d3f4477a5f9a251b5 on nskins:master.

scosbet commented 6 years ago

Thanks for the feedback @nskins. I found it really useful!

Sorry for the delay in getting back to this, it's been a busy couple of months!

My latest commits address all of the outstanding points you've raised as well as a couple of other minor improvements I noticed.

If there is anything else you'd like looked at or if you have any more comments, please let me know!

nskins commented 6 years ago

Awesome work on this issue, @scosbet! One very small bug remains, which is that when the player escapes, the player dies. I can fix it myself. It is a very simple fix so, if you're interested, check the upcoming two commits on master for an updated test (to expose it) and the bug fix itself.

I am merging your code now. Thanks again very much for this great contribution!

scosbet commented 6 years ago

Ah great catch! I hadn't considered that case, thanks for the fix 👍

My pleasure for contributing, really enjoyed it, I'll hopefully be back with more contributions soon!