-
Notifications
You must be signed in to change notification settings - Fork 90
Attachments and configurable blobs #532
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…bs and attachments
1 similar comment
…everal previous releases)
…ajoint-python into attachments Add the download_path attribute in fetch
datajoint/__init__.py
Outdated
|
|
||
| __author__ = "Dimitri Yatsenko, Edgar Y. Walker, and Fabian Sinz at Baylor College of Medicine" | ||
| __date__ = "Nov 15, 2018" | ||
| __date__ = "January 14, 2018" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean 2019?
datajoint/declare.py
Outdated
| if store_name[0] != 'external': | ||
| raise DataJointError('External store types must be specified as "external" or "external-<name>"') | ||
| if store_name[0] not in ('external', 'blob', 'attach'): | ||
| raise DataJointError('Invalid attribute type in:\n%s' % line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous error message gave a more explicit indication of what to do to fix the problem. I find the new one less informative.
| warnings.warn('ERD functionality depends on matplotlib and pygraphviz. Please install both of these ' | ||
| 'libraries to enable the ERD feature.') | ||
| warnings.warn('ERD functionality depends on matplotlib, networkx, and pygraphviz. ' | ||
| 'Please install both of these libraries to enable the ERD feature.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are now three libraries mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
|
|
||
|
|
||
| def test_heading(): | ||
| heading = modu.Simple().heading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You removed the () in line 8 of test_external_class.py above. Should you do so here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not really matter. Accessing attributes of classes directly still triggers instantiation of an object, so a more efficient way is to instantiate. The omission of parentheses was provided for user convenience.
|
|
||
| def test_insert_and_fetch(): | ||
| original_list = [1, 3, 8] | ||
| modu.Simple().insert1(dict(simple=1, item=original_list)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above; should the () be removed throughout this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is inside legacy tests. We support both syntaxes.
|
|
||
|
|
||
| def test_populate(): | ||
| image = modu.Image() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove these () throughout this and the next two methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we could but, when using the same table multiple times, it's more efficient to instantiate once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This particular snippet is inside test_legacy_external_class, which tests old functionality, so it's a good place to test whether the old syntax still works.
datajoint/version.py
Outdated
| @@ -1 +1 @@ | |||
| __version__ = "0.11.1" | |||
| __version__ = "0.12.0" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change this to "0.12.dev"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve for Dev release
dj.get_schema_names()