Use pytest, put tests into separate dir

bastikr / boolean.py

Implements boolean algebra in one module.

BSD 2-Clause "Simplified" License

78 stars 34 forks source link

Use pytest, put tests into separate dir #70

Open alisianoi opened 7 years ago

alisianoi commented 7 years ago

I am having some problems with batavia so I decided to sidestep a little and just shuffle things around in boolean.py, will later be getting back to making batavia work.

A thing to watch out for in this PR: readme display on pypi website might break because they use a rather strict and custom (afaik) parser for .rst and .md there. Once this lands, I will keep an eye on that and fix it if need be.

pombredanne commented 7 years ago

Is the init.py in https://github.com/bastikr/boolean.py/pull/70/files#diff-498cf53d35427897613cdfc4b76fc6ea really needed?

alisianoi commented 7 years ago

Sure, I can first do the changes in the same file and in the same order so that it would be easier to review. However, please note that you could:

instead of clicking on "Files changed" to see all the changes at once, click on the individual commits. That pulls up the diff for the exact test I was moving around/modifying.
do a similar thing locally in console by git diff hash0..hash1. If you pick hashes of consecutive commits, you see exactly the difference between them.

Anyway, I totally understand that this PR is big/hard to follow. As you suggest, I can use it to quick-start another PR that would first do the changes and then (once you've seen the exact changes), splits the files.

alisianoi commented 7 years ago

Is the init.py in https://github.com/bastikr/boolean.py/pull/70/files#diff-498cf53d35427897613cdfc4b76fc6ea really needed?

Removing the file and runnign pytest gives an import error. Tell me if you do not like the empty __init__.py, maybe it is possible to remove it and modify the imports somehow.

alisianoi commented 7 years ago

And I merged back the test_symbol.py, test_not.py, and test_base_element.py. Hope it makes things easier to review.

Did not merge back mock_advanced_algebra.py and mock_custom_algebra.py because the whole point was to put those classes into separate mock files.

Also did not merge back test_parse.py because those are new tests for the parse functionality, so I am writing them in a separate file.

Also rewrote README.md as README.rst, added some badges and checked it with restview and python setup.py check --restructuredtext -- seems ok.

Update: the things I did not do yet:

write a second test that uses parse as before, https://github.com/bastikr/boolean.py/pull/70#discussion_r122733188

alisianoi commented 7 years ago

Everything has been addressed, I am removing the [WIP] prefix and waiting for a review.

alisianoi commented 7 years ago

Ok, I have addressed:

The two places where I removed tests. They are now back with explicit exception checks.
The not and missing (), I have written explicit ()
Squashed the spaces in ~ ~~ into ~~~

For you to decide:

why pytest: https://github.com/bastikr/boolean.py/pull/70#discussion_r125045133
why whitespace: https://github.com/bastikr/boolean.py/pull/70#discussion_r125047310
(not) to split the big one: https://github.com/bastikr/boolean.py/pull/70#discussion_r125078228
the xfail'ed IndexError: https://github.com/bastikr/boolean.py/pull/70#discussion_r125079649

Also, when answering your questions, I misclicked the "Start Review" button (or something) and my last three comments ended up as a separate review. I cannot "dismiss" it, so if you could that would be great. Also, I am new to the review mechanic on github so please tell me when I am doing things wrong.

Will be removing the [WIP] and waiting for your review.