Skip to content

Change 'ancestor_of'/'descendant_of' to 'with_descendants'/'with_ancestors'#2278

Merged
sphuber merged 3 commits into
aiidateam:provenance_redesignfrom
ConradJohnston:fix_2209_replace_ancestor_of
Dec 4, 2018
Merged

Change 'ancestor_of'/'descendant_of' to 'with_descendants'/'with_ancestors'#2278
sphuber merged 3 commits into
aiidateam:provenance_redesignfrom
ConradJohnston:fix_2209_replace_ancestor_of

Conversation

@ConradJohnston
Copy link
Copy Markdown
Contributor

Fixes #2209 .

Deprecates the Querybuilder 'ancestor_of'/'descendant of' join type specifications and replaces them with 'with_descendants'/'with_ancestors' respectively. All deprecated uses are updated and the docs changed to reflect this new convention.

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 3, 2018

Coverage Status

Coverage increased (+6.9%) to 68.99% when pulling a8d5227 on ConradJohnston:fix_2209_replace_ancestor_of into 1adafc4 on aiidateam:provenance_redesign.

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.

I've got a couple of comments/questions

Comment thread aiida/orm/querybuilder.py Outdated
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.

If the user uses the old 'descendant_of', will this still work? Maybe we should keep all four options for now?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, well spotted. Put these back in.

Comment thread aiida/orm/querybuilder.py Outdated
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.

Is this _beta version still implemented? Probably @lekah can answer this

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No, what we're doing now (transitive closure on the fly) used to be called _beta. This string passed is just for the exception, and can be changed without a problem (i.e. remove _beta in the string).
My suggest: without a new issue within this pull request, if noone objects.

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.

Good for me!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Made these changes.

- Deprecate ancestor_of and descendant_of
- Replace all occurances of ancestor_of with with_descendants
- Replace all occurances of descendant_of with with_ancestors
- Update docs with this convention
- Remove _beta versions of ancestor_of and descendant_of joins
- Add ancestor_of and descendant_of back in to check for valid join keywords.
@ConradJohnston ConradJohnston force-pushed the fix_2209_replace_ancestor_of branch from ca412b7 to 8758769 Compare December 4, 2018 08: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.

Great!

@sphuber sphuber merged commit af61105 into aiidateam:provenance_redesign Dec 4, 2018
@ConradJohnston ConradJohnston deleted the fix_2209_replace_ancestor_of branch December 5, 2018 09:02
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.

5 participants