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.

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

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

Conversation

ConradJohnston
Copy link
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

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
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

@@ -1659,7 +1661,7 @@ def _build(self):
isouterjoin = verticespec.get('outerjoin')
edge_tag = verticespec['edge_tag']

if verticespec['joining_keyword'] in ('descendant_of', 'ancestor_of'):
if verticespec['joining_keyword'] in ('with_ancestors', 'with_descendants'):
Copy link
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
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.

@@ -1262,7 +1262,7 @@ def _join_descendants_recursive(self, joined_entity, entity_to_join, isouterjoin
"""

self._check_dbentities((joined_entity, self._impl.Node), (entity_to_join, self._impl.Node),
'descendant_of_beta')
'with_ancestors_beta')
Copy link
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
Contributor

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
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
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.
Copy link
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.

None yet

5 participants