search

tmwilder / pogoiv

MIT License
13 stars 4 forks source link

Appraisal #17

Closed Arkaivos closed 8 years ago

tmwilder commented 8 years ago

Thank you for the pull request! Taking a look at it now.

tmwilder commented 8 years ago

Except for the more minor styling comments, this looks good.

One thing I would ask if you've got the time is to throw together a few test cases that run through your added functionality, check out:

https://docs.python.org/2/library/unittest.html for a top level summary

and

https://github.com/tmwilder/pogoiv/blob/master/pogoiv/test/test_base_stats.py for a specific example.

if you aren't familiar with Python's unittest library. Since this is a hobby project the bar is a bit lower than production code for tests, but they still drastically improve ability to upkeep it over time.

I'd briefly throw down coverage for:

  1. Does your filter behave as expected given one happy path known matching input and one known not matching input?
  2. Do edge cases like values right on the experience borders do what you were expecting?
  3. Does the whole shtick work as expected given one known valid input that includes appraisal data?

Let me know if/how I can help, I'm excited to get this merged in.

This is a great PR! Thank you again for opening it.

Arkaivos commented 8 years ago

Thank you for your comments! I'll take a look at it tomorrow and I'll make the proposed changes! It's time to sleep today :)

PD: Yes, it's percentage, sorry. English is not my first language and sometimes I make these mistakes :P

2016-09-05 22:08 GMT+02:00 Timothy Wilder notifications@github.com:

Except for the more minor styling comments, this looks good.

One thing I would ask if you've got the time is to throw together a few test cases that run through your added functionality, check out: https://github.com/tmwilder/pogoiv/blob/master/pogoiv/ test/test_base_stats.py if you aren't familiar with Python's unittest library.

I'd briefly throw down coverage for:

  1. Does your filter behave as expected given one happy path known matching input and one known not matching input?
  2. Do edge cases like values right on the experience borders do what you were expecting?
  3. Does the whole shtick work as expected given one known valid input that includes appraisal data?

Let me know if/how I can help, I'm excited to get this merged in.

This is a great PR! Thank you again for opening it.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tmwilder/pogoiv/pull/17#issuecomment-244806117, or mute the thread https://github.com/notifications/unsubscribe-auth/AU_ZARlS2Y5w3fXaEtMnTdUKAkoq7GmPks5qnHbCgaJpZM4J1RMZ .

tmwilder commented 8 years ago

Looks good, nice work!

Arkaivos commented 8 years ago

Great! Thanks! :)

2016-09-08 8:19 GMT+02:00 Timothy Wilder notifications@github.com:

Merged #17 https://github.com/tmwilder/pogoiv/pull/17.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tmwilder/pogoiv/pull/17#event-781928742, or mute the thread https://github.com/notifications/unsubscribe-auth/AU_ZAWvXzT36drsxv9Yooe1dkez5tTsSks5qn6kAgaJpZM4J1RMZ .