Cartesian products #25
Conversation
|
@nedbat @doctoryes: Review? |
|
|
| @data(1, 2, 3) | ||
| @data(4, 5, 6) | ||
| def test_products(self, first_value, second_value): | ||
| self.assertTrue(first_value < second_value) |
There was a problem hiding this comment.
How about testing for the number of products expected? It'd be nice if these three tests also told someone perusing the code what to expect as far as what products result from the nesting.
There was a problem hiding this comment.
Perhaps this wouldn't be needed if there was some documentation elsewhere about what to expect when nesting @DaTa and @file_data decorators.
|
Hey thanks for this contribution, and sorry for the late reply. I will need to review this in more detail for feedback on code specifics. I'd appreciate an addition to the documentation describing how this feature is supposed to work. Also a green build would be nice (apparently only a question of pep8 compliance) ;) |
| @@ -63,7 +63,7 @@ def hello(): | |||
|
|
|||
| pre_size = len(hello.__dict__) | |||
| keys = set(hello.__dict__.keys()) | |||
| data_hello = data(1, 2)(hello) | |||
| data_hello = data(1, 2)(data(3)(hello)) | |||
There was a problem hiding this comment.
I'd prefer adding a new test case for the new feature, rather than overloading an existing test case.
The main reason being, any new feature should strive to be backwards compatible... meaning all existing tests should pass unmodified. It is harder for me to figure out whether your contribution is backwards-compatible or not, if you modify existing tests.
|
I like that you have brought more structure to the code. This had started out as a tiny library but upon growing with contributions it was starting to look messy. Much appreciated! |
|
@txels: I've updated my PR based on your comments. |
|
|
| @@ -4,7 +4,7 @@ envlist = py27, py33, py34 | |||
| [testenv] | |||
| deps = | |||
| nose | |||
| pep8 | |||
| pep8>=1.5.7,<1.6 # Versions limited to work with flake8 | |||
There was a problem hiding this comment.
This was fixed in another PR, which unfortunately now makes yours conflict.
|
Sorry for the delay. I may be able to spend some time on this tomorrow. |
|
Hey @cpennington I just received this other PR from @mycharis that tackles the same issue: #29 I will eventually have to choose one or the other, would you mind taking a look at how that one compares to yours? Maybe you guys could discuss the pros and cons of each approach and come up with the preferred solution. |
|
Closing this in favor of #29, which seems to have built on the ideas that I started with, and taken them to a better place. |
This allows you to nest
@dataand@file_datadecorators to produce Cartesian products.