search

roguerb / pull-request-workshop

0 stars 6 forks source link

Fix all the things #15

Closed mistertime closed 8 years ago

mistertime commented 8 years ago

Fixes #13, #12, #11, #9, and #2, as well as several others that may have been closed. I have also taken the liberty of designing new tests for each method, which I have ensured are incalculably more robust than the originals.

conroywhitney commented 8 years ago

Hey @mistertime. Thanks for finding/fixing all of these issues.

However, due to the size/scope of this Pull Request, unfortunately we cannot merge it in like this. Could you break / cherry-pick some of these fixes out into their own Pull Requests with corresponding Issues? Keeping each change as small as possible really helps ensure the best outcome for the project.

Also, just one note regarding the tests -- while using 2 everywhere was obviously a terrible idea (my bad!), I'm not a huge fan of using random numbers in tests. That can introduce nondeterministic outcomes, which can lead to intermittent test failures, which are just terrible to debug. Some well-chosen input variables would be much preferred.

Thanks!

conroywhitney commented 8 years ago

Closing RE: #23. Thanks @mistertime!