Skip to content

Prevent to store base Nodes in the DB but only their subclasses#2301

Merged
sphuber merged 2 commits into
aiidateam:provenance_redesignfrom
giovannipizzi:fix_unstorable_base_nodes
Dec 6, 2018
Merged

Prevent to store base Nodes in the DB but only their subclasses#2301
sphuber merged 2 commits into
aiidateam:provenance_redesignfrom
giovannipizzi:fix_unstorable_base_nodes

Conversation

@giovannipizzi
Copy link
Copy Markdown
Member

This PR fixes #2300, that explains the reasoning behind it

@giovannipizzi giovannipizzi requested a review from sphuber December 6, 2018 08:59
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wrong comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wrong comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion:
Note that here we use `@workfunctions` and `@calculations`, the concrete versions of the `@process_function` decorator, even though we are testing only the shared functionality that is captured in the `@process_function` decorator, relating to the transformation of the wrapped function into a `FunctionProcess`. The reason we do not use the `@process_function` decorator itself, is because it does not have a node class by default. We could create one on the fly, but then anytime inputs or outputs would be attached to it in the tests, the `validate_link` function would complain as the dummy node class is not recognized as a valid process node.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe put this in a function revert_database_schema such that you can deduplicate the code in the tearDown method

The main work is actually fixing all tests to avoid to use Node
directly. This also allowed to spot a few bugs and inconsistencies
in the tests.
@giovannipizzi giovannipizzi force-pushed the fix_unstorable_base_nodes branch from 9d21b40 to d6e5e4c Compare December 6, 2018 11:17
@giovannipizzi
Copy link
Copy Markdown
Member Author

I took care of all 4 comments and rebased

@sphuber sphuber merged commit 06d06bc into aiidateam:provenance_redesign Dec 6, 2018
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 8, 2018

Coverage Status

Coverage decreased (-0.07%) to 68.938% when pulling cee46ee on giovannipizzi:fix_unstorable_base_nodes into 7bab498 on aiidateam:provenance_redesign.

@giovannipizzi giovannipizzi deleted the fix_unstorable_base_nodes branch April 16, 2019 10:43
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.

3 participants