Fix issues #483, #516, and #518#517
Conversation
…. Also rename class Query -> Expression
eywalker
left a comment
eywalker
left a comment
There was a problem hiding this comment.
Test seems to have failed due to changes in the test conditions. Please check and ensure test runs again.
datajoint/expression.py
Outdated
| @@ -819,7 +819,7 @@ class U: | |||
|
|
|||
| In aggregation, dj.U is used to compute aggregate expressions on the entire relation. | |||
There was a problem hiding this comment.
These doc strings need to be updated to use the new terminology (i.e. no relation)
| D().populate() | ||
| E().populate() | ||
| Experiment().populate() | ||
| A.insert(A.contents, skip_duplicates=True) |
There was a problem hiding this comment.
We should mix in instance based inserts as that's supposed to work too. In general, please do not remove test cases but rather just add.
There was a problem hiding this comment.
Yes, we have a good mixture. If you review the tests, the majority of tests still instantiate the classes.
There was a problem hiding this comment.
As in every operation that's supposed to work for either instance or class ought to be tested on both. And it still holds that in general you shouldn't just be replacing previous test cases if they are supposed to continue to work.
There was a problem hiding this comment.
I should add that I realize that this is setup so perhaps it was a bad piece to grab onto to add these notes. May be we should just add a distinct test case that specifically tests the equivalence of class method and instance method calls so that we don't have to worry about using both in all test cases.
There was a problem hiding this comment.
This is not meant for testing, just the regular setup. It works.
There was a problem hiding this comment.
We do have testing for both instances and classes.
tests/__init__.py
Outdated
| for db in cur.fetchall(): | ||
| conn.query('DROP DATABASE `{}`'.format(db[0])) | ||
| conn.query('SET FOREIGN_KEY_CHECKS=1') | ||
| conn.query('SET FOREIGN_KEY_CHECKS=1').close() |
There was a problem hiding this comment.
Are you sure that addition of .close() is a safe operation? The connection object is largely shared throughout the test so we have to ensure that it doesn't get closed in the middle.
There was a problem hiding this comment.
This does not close the connection but the cursor. However, I experimented with closing the cursor after every query to attempt to address #515 but I do not think that was the problem. I took all .close() out but forgot this one.
There was a problem hiding this comment.
Each query creates its own cursor so closing the cursor should not make any difference
… what was breaking Travis
…s from __init__ in nosetests
…t with "make_module_code"
3 participants
This pull request follows PR #514
Fixes #483, #516, #518
.projto preserve the original order of attributesQuery->Expressionandquery.py->expression.py