Clean up and test the refactor before merging into develop

Cobular commented 5 years ago

Hey all, I really should have done this in #5 , but I merged that before fully testing. Here, I am going to add more things before I merged it in. Notably:

[x] Documentation on what each file is for
[x] Thorough testing of each file, will use code coverage or something to test
[x] Possibly even more refactoring? May break things down into folders now
[x] Confirm that this can be made into a package and still work

JosephFKnight commented 5 years ago

@JakeCover I have those docstrings ready, can I get push access to this branch?

Cobular commented 5 years ago

@JakeCover I have those docstrings ready, can I get push access to this branch?

Shoot! I just added some docstrings, can you add yours to make them better? You are now a collaborator, sorry I forgot! .

Cobular commented 5 years ago

Thank you for fixing that on 67eda55. Also, 5bb0c51 is a MUCH more elegant solution to my quick and dirty approach. I explained some of my reasoning in https://github.com/JakeCover/distest/issues/8#issuecomment-496264079, but I wrote most of that before seeing this change. It basically boils down to a quick fix, yours is much nicer. Since some of these changes are a bit complicated (977b3c9), although a huge improvement, make sure you describe the ways things now work in docstrings. Thank you!

JosephFKnight commented 5 years ago

Just noticed _run_by_predicate isnt documented and you're right its not immediately obvious what it does. I'll doc it this afternoon.

On Mon, May 27, 2019, 11:50 AM J Cover notifications@github.com wrote:

Thank you for fixing that on 67eda55 https://github.com/JakeCover/distest/commit/67eda557e4f141b10483b6972e445db31cf190db. Also, 5bb0c51 https://github.com/JakeCover/distest/commit/5bb0c514c40991f92dc6ddb8911366389a582e52 is a MUCH more elegant solution to my quick and dirty approach. I explained some of my reasoning in #8 (comment) https://github.com/JakeCover/distest/issues/8#issuecomment-496264079, but I wrote most of that before seeing this change. Since some of these changes are a bit complicated (977b3c9 https://github.com/JakeCover/distest/commit/977b3c9e99516d80be70b380913df66e7882cdd0), although a huge improvement, make sure you describe the ways things now work in docstrings. Thank you!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JakeCover/distest/pull/12?email_source=notifications&email_token=AK6KUYITAQMXLNHG35X2RJ3PXQGO3A5CNFSM4HPSRFN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWKGPNA#issuecomment-496265140, or mute the thread https://github.com/notifications/unsubscribe-auth/AK6KUYLMHVG7N3PSONDHAJLPXQGO3ANCNFSM4HPSRFNQ .

Cobular commented 5 years ago

Ok, I have it working. The tests pass! I'm pretty sure the other Travis CI is a duplicate and should be removed eventually.

JosephFKnight commented 5 years ago

Great, what next? merge into develop?

On Mon, May 27, 2019, 10:20 PM J Cover notifications@github.com wrote:

Ok, I have it working. The tests pass! I'm pretty sure the other Travis CI is a duplicate and should be removed eventually.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/JakeCover/distest/pull/12?email_source=notifications&email_token=AK6KUYI4HJWUVT3SEHXDT6TPXSQH7A5CNFSM4HPSRFN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWK3NBQ#issuecomment-496350854, or mute the thread https://github.com/notifications/unsubscribe-auth/AK6KUYPSXYUCZ2WG3GN54VDPXSQH7ANCNFSM4HPSRFNQ .

Cobular commented 5 years ago

Yeah, I'll do that.

Cobular / distest

Clean up and test the refactor before merging into develop #12