-
Notifications
You must be signed in to change notification settings - Fork 187
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
Export/import of extras: #2416
Export/import of extras: #2416
Conversation
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.
I proposed some changes. As an additional comment, if I understand correctly, the extras import mode only affects nodes that are already present in the database. Specifically the none
option will not import any of the extras in the archive. But shouldn't there also be a general option to not import any extras, regardless of whether the node is already present in the database or not?
I wanted to discuss with you this "general option" to import extras. I see two ways:
I would prefer to not define one option that will affect both existing and new nodes, because in this way you can not, for example, import new extras for the existing nodes and do not import extras for the new nodes. Of course, people may never need that, but I think if to add a new option it should allow to any combination of behavior with regard to the new and existing nodes. |
Apart from that last issue, I've already implemented all your suggestions |
I agree that we probably want to have a solution that allows the user to choose all possible options, so that would exclude the first one. I would be fine with adding an additional option that exclusively effects the import behavior of extras for new nodes. @giovannipizzi what do you think? |
Also, for the moment all extras are exported. Let me know I should add yes/no option for the extras export. |
I spoke to @giovannipizzi and he agrees that we should have all combinations available to (power) users. So we should have two flags, So if you can change the name of the current flag and implement the second |
@sphuber all python 2 tests failed because of the same error:
and this has nothing to do with my changes |
Yeah there seems to be a problem with |
Found the problem: the package |
@yakutovicha I opened a PR to fix the problem, so once it is merged you can rebase so your tests will run again |
thanks @sphuber, just merged your changes |
fixes #1761 1) Extras are now exported together with other data 2) Extras are fully imported if the corresponding node did not exist 3) In case imported node already exists in a database the following logic may be chosen to import the extras: - keep_existing (default): keep extras with different names (old and impoted ones), keep old value in case of name collision - update_existing: -/-/-, keep new value in case of name collision - ask: -/-/-, ask what to do in case of the name collision - mirror: completely overwrite the extras by the imported ones - none: keep old extras
1) two options: --extras-mode-new and --extras-mode-existing 2) skip hidden extra for codes and all extras starting with _aiida_
1) Except when it is not a string 2) Except when it is too short or too long 3) add some tests for those cases 4) Improve related error messages
I made the requested changes. @sphuber and @szoupanos let me know if something else still needs to be done |
Thanks a lot @yakutovicha great work |
fixes #1761
logic may be chosen to import the extras:
ones), keep old value in case of name collision