Skip to content

Renaming DbNode.type to DbNode.node_type#2517

Closed
sphuber wants to merge 2 commits into
aiidateam:provenance_redesignfrom
sphuber:fix_2510_rename_node_type_to_node_type
Closed

Renaming DbNode.type to DbNode.node_type#2517
sphuber wants to merge 2 commits into
aiidateam:provenance_redesignfrom
sphuber:fix_2510_rename_node_type_to_node_type

Conversation

@sphuber
Copy link
Copy Markdown
Contributor

@sphuber sphuber commented Feb 22, 2019

Fixed #2510

While we are at it....

giovannipizzi
giovannipizzi previously approved these changes Feb 22, 2019
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.

So you did manage to do it!! :-)

@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Feb 22, 2019

@giovannipizzi faster than I anticipated. I could start working on #2398 . Originally we discussed that it would not be critical, but seeing as we still have a week and I could get started on it now, might be worth it to do it before the beta release. What do you think?

Also the migrations here were dead easy. Are we sure we are not missing anything (not including the migration of export archives)?

@coveralls
Copy link
Copy Markdown

coveralls commented Feb 22, 2019

Coverage Status

Coverage decreased (-0.09%) to 69.63% when pulling 2de803bcb142b1065faf3001d8b8154cc2ea5847 on sphuber:fix_2510_rename_node_type_to_node_type into f8c0191 on aiidateam:provenance_redesign.

@sphuber sphuber force-pushed the fix_2510_rename_node_type_to_node_type branch from 2de803b to 26e426d Compare February 22, 2019 18:28
@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Feb 22, 2019

Uhm.... there is a reason it went so fast. I forgot to un-comment the migration tests 🤦‍♂️
but of course those are failing miserably.

@giovannipizzi
Copy link
Copy Markdown
Member

I think the migrations are ok (we're getting too used to complex ones :-D )
Ok to proceed to #2398 (a bit more complex, requires again a migration), but then I suggest to prioritise stating cleaning up the wiki page for backward-incompatible things (I just reopened #2320)

@sphuber
Copy link
Copy Markdown
Contributor Author

sphuber commented Feb 23, 2019

This might not actually be reasonably doable after all. Changing the DbNode model by changing the type column's name breaks all the migration tests. The actual database model can no longer be used anywhere, which therefore includes our backend and frontend classes. The only solution is to only use the apps to get the model at the time of the migration that is being tested. This is doable for quite a few tests and I have already migrated them to only use the django apps. But there are three TestCalcAttributeKeysMigration, TestDuplicateNodeUuidMigration and the TestTrajectoryDataMigration that use attributes, the repository and trajectory array setters and getters, that are not clear how to decouple from our ORM. At this point I am not sure whether the required effort would be worth it, even though I think the name change is important. Any ideas @giovannipizzi ?

@sphuber sphuber closed this Feb 26, 2019
@sphuber sphuber deleted the fix_2510_rename_node_type_to_node_type branch February 26, 2019 21:18
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