Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Export/import of comments (#1760) #2413

Merged
merged 1 commit into from
Feb 8, 2019
Merged

Export/import of comments (#1760) #2413

merged 1 commit into from
Feb 8, 2019

Conversation

CasperWA
Copy link
Contributor

@CasperWA CasperWA commented Jan 22, 2019 •
edited
Loading

This fixes #1760.

Comments can now be exported/imported using export-version 0.4.

At this stage of the implementation, all comments pertaining to a node chosen for export will be exported.
Comments cannot be exported by themselves, nor is a Comment a valid entity to export directly, i.e. comments can only be exported indirectly through nodes.

There is a suggestion to add a flag to ex-/include comments when exporting.

If multiple users add comments to a node, all users will be exported recursively.

Examples of how data.json and metadata.json might look have been updated with Comment in the documentation.

An additional pair of joins has been added to QueryBuilder:

  • with_comment to join a Comment to a User
  • with_user to join a User to a Comment

These, along with a similar pair of joins for Node, has been added to the table of Joining entities in the documentation.

Lastly, Comment can now be imported directly from aiida.orm.

@CasperWA CasperWA added this to the v1.0.0b1 milestone Jan 22, 2019
@CasperWA CasperWA self-assigned this Jan 22, 2019
@coveralls
Copy link

coveralls commented Jan 22, 2019 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 69.669% when pulling dfa7ccc on CasperWA:fix_1760_export_import_comments into a000899 on aiidateam:provenance_redesign.

@CasperWA
Copy link
Contributor Author

Latest commit adds support for excluding export of Comments.

Using the flag include_comments=False when using export(), Comments, and any associated Users that were not already related in the export, are excluded from the export.

The same flag is added to verdi export create in the following way:
"--include-comments/--exclude-comments", "-c / -xc"

The default is to include Comments (and any associated User entities not already included through other exported entities).

sphuber
sphuber requested changes Jan 29, 2019
View reviewed changes
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

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

Some minor changes, but otherwise looks good

aiida/backends/tests/orm/comments.py Outdated Show resolved Hide resolved
aiida/cmdline/commands/cmd_export.py Outdated Show resolved Hide resolved
docs/source/import_export/main.rst Show resolved Hide resolved
@sphuber
Copy link
Contributor

sphuber commented Jan 30, 2019

This has gone through review and after other blocking PRs are resolved I will rebase and merge

szoupanos
szoupanos previously requested changes Feb 4, 2019
View reviewed changes
Copy link
Contributor

@szoupanos szoupanos left a comment

Choose a reason for hiding this comment

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

Just modify a bit the test to make sure that you export also users that did a comment on a node that was not created by them (the user that created a node will be exported anyway).

aiida/backends/tests/export_and_import.py Show resolved Hide resolved
@szoupanos
Copy link
Contributor

I would not merge provenance_redesign' into your branch but rebase your branch to accommodate the changes of provenance_redesign.

@CasperWA
Copy link
Contributor Author

CasperWA commented Feb 5, 2019

I would not merge provenance_redesign' into your branch but rebase your branch to accommodate the changes of provenance_redesign.

Sure. I 'accidentally' pressed the "Update branch" button :)
But in the end it should all be squashed and rebased in any case. So I do not think it is an issue.

@sphuber
Copy link
Contributor

sphuber commented Feb 8, 2019

@CasperWA In this case it still is less convenient because there are merge conflicts that I had to resolve locally. Due to the merges I couldn't simply squash your commits and then do a single rebase. Instead I had to merge it. I hope when we squash merge it will still be attributed to you, but I am not sure what will happen since the last commit is now mine, despite it being just a merge commit.

@sphuber sphuber force-pushed the fix_1760_export_import_comments branch 3 times, most recently from 088b3da to 4c2f591 Compare February 8, 2019 15:02
@CasperWA @sphuber
Add support for the export/import of Comment entities
dfa7ccc 
To make the export of `Comment` entities possible, the `QueryBuilder` had to
be extended to support retrieving comments for a given `Node`. The
following changes were applied:

 * Added support to join user to comment (`with_user`)
 * Added support to join comment to user (`with_comment`)

The `verdi export create` has a new flag to include or exclude the
export of comments for nodes that are to be exported, defaulting to include:

 `--include-comments/--exclude-comments`

Documentation has been updated to include new `QueryBuilder` join args
in table and the `metadata.json` example has been updated in documentation
to include correct Comment info.
@sphuber sphuber force-pushed the fix_1760_export_import_comments branch from 4c2f591 to dfa7ccc Compare February 8, 2019 15:38
@sphuber sphuber dismissed szoupanos’s stale review February 8, 2019 16:08

Comments have been addressed

@sphuber sphuber merged commit 0c9e0b8 into aiidateam:provenance_redesign Feb 8, 2019
@sphuber
Copy link
Contributor

sphuber commented Feb 8, 2019

Great success, thanks a lot for the great work @CasperWA !

@sphuber sphuber mentioned this pull request Feb 8, 2019
Closed
3 tasks
@CasperWA CasperWA deleted the fix_1760_export_import_comments branch February 14, 2019 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szoupanos szoupanos szoupanos left review comments

@sphuber sphuber sphuber approved these changes

Assignees

@szoupanos szoupanos

@CasperWA CasperWA

Labels
priority/nice-to-have topic/archive
Projects
None yet
Milestone
v1.0.0b1
Development

Successfully merging this pull request may close these issues.
4 participants
@CasperWA @coveralls @sphuber @szoupanos