Skip to content

Conversation

@dimitri-yatsenko
Copy link
Member

@dimitri-yatsenko dimitri-yatsenko commented Dec 3, 2018

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 90.025% when pulling 2c04d74 on dimitri-yatsenko:attachments into ebfbcd4 on datajoint:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.8%) to 90.025% when pulling 2c04d74 on dimitri-yatsenko:attachments into ebfbcd4 on datajoint:master.

@coveralls
Copy link

coveralls commented Dec 3, 2018

Coverage Status

Coverage decreased (-0.5%) to 90.34% when pulling afeadb1 on dimitri-yatsenko:attachments into 2d21899 on datajoint:dev.

@dimitri-yatsenko dimitri-yatsenko dismissed eywalker’s stale review January 16, 2019 06:26

Fixed 3.4 compatibility


__author__ = "Dimitri Yatsenko, Edgar Y. Walker, and Fabian Sinz at Baylor College of Medicine"
__date__ = "Nov 15, 2018"
__date__ = "January 14, 2018"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean 2019?

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)
Copy link
Contributor

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.')
Copy link
Contributor

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.

Copy link
Member Author

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
Copy link
Contributor

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?

Copy link
Member Author

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))
Copy link
Contributor

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?

Copy link
Member Author

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()
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member Author

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.

@dimitri-yatsenko dimitri-yatsenko changed the base branch from master to dev February 7, 2019 21:40
@@ -1 +1 @@
__version__ = "0.11.1"
__version__ = "0.12.0"
Copy link
Contributor

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"

Copy link
Contributor

@eywalker eywalker left a 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

@eywalker eywalker merged commit 26fc8e1 into datajoint:dev Feb 8, 2019
This was referenced Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants