Skip to content

Move aiida.orm.data within the aiida.orm.node module#2402

Merged
sphuber merged 1 commit into
aiidateam:provenance_redesignfrom
sphuber:fix_2200_move_orm_data
Feb 8, 2019
Merged

Move aiida.orm.data within the aiida.orm.node module#2402
sphuber merged 1 commit into
aiidateam:provenance_redesignfrom
sphuber:fix_2200_move_orm_data

Conversation

@sphuber
Copy link
Copy Markdown
Contributor

@sphuber sphuber commented Jan 18, 2019

Fixes #2200

This final move makes the hierarchy of all node modules and its sub
classes consistent. The Data and ProcessNode classes who are both
sub classes of the Node base class and are direct siblings live on the
same level in the hierarchy, direct within the aiida.orm.node module.

Note that because the type string of all Data nodes now will start
with node. instead of data., a data migration had to be added.

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 18, 2019

Coverage Status

Coverage decreased (-0.03%) to 69.682% when pulling c4bd515 on sphuber:fix_2200_move_orm_data into 631ff01 on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2200_move_orm_data branch from 68cc805 to 43ba8a9 Compare January 19, 2019 08:03
@sphuber sphuber changed the title [BLOCKED] Move aiida.orm.data within the aiida.orm.node module Move aiida.orm.data within the aiida.orm.node module Jan 19, 2019
@sphuber sphuber requested a review from giovannipizzi January 19, 2019 08:15
@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Jan 19, 2019

@giovannipizzi this PR is good for review. I added the suggestion you made in #2401 to add a message to the load_node_class message in the case of an ununderstood type string. Again most of the work in this PR is moving files around and changing imports. The only really important thing to check are the migrations that I added. Please make sure that I got the SQL correct. I didn't have time to add actual tests. Feel free to add those before merging if you like, maybe you can do this with Conrad in the coding week.

FInally, the test at Jenkins is failing because of the bug we had before. I already opened this PR before #2401 was closed, so now I had to rebase this, but that leads to merging conflicts on the Jenkins server, because it does not automatically do a hard reset. If you can access the machine, you can run git reset HEAD --hard in the workspace of this PR, which should make it work again. I just don't think I have access

@giovannipizzi
Copy link
Copy Markdown
Member

Mmm, I think the SQL is wrong (^ as line-start is valid for regex but not for LIKE, that only accepts % and _ - not putting a % at the beginning implicitly means "starts with").
I think then that it's best to wait and add a couple of tests - I'll work on the other issues and if I've time I'll add tests here. I hope that this will not create too many conflicts with the Node class migration to the new infrastructure...

Copy link
Copy Markdown
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

At least the SQL needs to be adapted (see my comment above), I didn't do a full review yet.

@sphuber sphuber force-pushed the fix_2200_move_orm_data branch 2 times, most recently from f077e0c to 24823d0 Compare February 4, 2019 17:29
Copy link
Copy Markdown
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

All good

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A comment: while technically the regex alone would be not fully correct (it should be '^data\\.' I think, as . matches any character) it's ok because the following WHERE clause does the right filter

This final move makes the hierarchy of all node modules and its sub
classes consistent. The `Data` and `ProcessNode` classes who are both
sub classes of the `Node` base class and are direct siblings live on the
same level in the hierarchy, direct within the `aiida.orm.node` module.

Note that because the type string of all `Data` nodes now will start
with `node.` instead of `data.`, a data migration had to be added.
@sphuber sphuber force-pushed the fix_2200_move_orm_data branch from 28abc6a to c4bd515 Compare February 8, 2019 05:49
@sphuber sphuber merged commit dcdfbf2 into aiidateam:provenance_redesign Feb 8, 2019
@sphuber sphuber deleted the fix_2200_move_orm_data branch February 8, 2019 06:27
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