Code review comment for lp:~allenap/storm/value-columns-by-name

Revision history for this message
Jamu Kakar (jkakar) wrote :

[1]

Can you please add docstrings to the tests. Also, the @param for
'columns' in the docstring for ResultSet.values needs to be updated.

[2]

+ def test_find_values_by_column_name(self):

There are several different cases being exercised in this test. I
recommend you break it up into several smaller tests.

[3]

+ # If more than one column in the result set has the same name,
+ # the first will be chosen.
+ result = self.store.find(
+ (Foo.id, FooValue.id), FooValue.foo_id == Foo.id)
+ result = result.config(distinct=True).order_by(Foo.id)
+ values = result.values('id')
+ self.assertEquals(list(values), [10, 20])

I agree with James that this case should raise FeatureError. We
shouldn't be making guesses about which column the user is
specifying if the choice is ambiguous.

[4]

+ def test_find_multiple_values_by_column_name(self):

This test could also be split into two, so that each test exercises
one specific behaviour (multiple columns, and mixed string and Column
values).

review: Needs Fixing

« Back to merge proposal