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

Add the flat argument to QueryBuilder.all() method #3945

Merged

Conversation

sphuber
Copy link
Contributor

@sphuber sphuber commented Apr 19, 2020

Fixes #3943

This allows to simplify the commonly found form:

results = [entry[0] for entry in builder.all()]

to the more compact and readable form:

results = builder.all(flat=True)

The default is set to flat=False such that this change is fully
backwards compatible. Occurrences in the code base itself have been
updated as much as possible.

@codecov
Copy link

codecov bot commented Apr 19, 2020

Codecov Report

Merging #3945 into develop will decrease coverage by 0.08%.
The diff coverage is 94.44%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #3945      +/-   ##
===========================================
- Coverage    78.31%   78.23%   -0.09%     
===========================================
  Files          461      461              
  Lines        34082    34067      -15     
===========================================
- Hits         26692    26652      -40     
- Misses        7390     7415      +25     
Flag Coverage Δ
#django 70.27% <83.33%> (-0.09%) ⬇️
#sqlalchemy 71.13% <86.11%> (-0.09%) ⬇️
Impacted Files Coverage Δ
aiida/cmdline/commands/cmd_process.py 74.66% <0.00%> (ø)
aiida/cmdline/utils/common.py 65.42% <100.00%> (-0.13%) ⬇️
aiida/orm/entities.py 100.00% <100.00%> (ø)
aiida/orm/groups.py 92.75% <100.00%> (+0.28%) ⬆️
aiida/orm/implementation/django/logs.py 96.15% <100.00%> (ø)
aiida/orm/implementation/sqlalchemy/comments.py 94.87% <100.00%> (ø)
aiida/orm/implementation/sqlalchemy/logs.py 94.82% <100.00%> (ø)
aiida/orm/nodes/data/cif.py 87.12% <100.00%> (ø)
aiida/orm/nodes/data/code.py 85.24% <100.00%> (ø)
aiida/orm/nodes/data/upf.py 80.42% <100.00%> (ø)
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d13de0e...fd296ce. Read the comment docs.

Copy link
Member

@ramirezfranciscof ramirezfranciscof left a comment

Choose a reason for hiding this comment

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

Small but incredibly useful feature since it is probably the way in which you want the results of the QB for more than 80% of its applications. I only have two small comments/questions but I'll leave the seal of approval since these are very small and maybe don't need changing.

aiida/orm/groups.py Show resolved Hide resolved
@@ -629,6 +629,32 @@ def test_direction_keyword(self):
res2 = {item[1] for item in qb.all()}
self.assertEqual(res2, {d2.id, d4.id})

@staticmethod
def test_flat():
"""Test the `flat` keyword for the `.iterall()`, `.all()` and `.one()` methods."""
Copy link
Member

Choose a reason for hiding this comment

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

Mmm how is this testing .iterall() and .one()? Is this some copy/paste leftover?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is just a mistake that I left in. I originally thought I was going to implement it for all, but for one it would break backwards-compatibility and for iterall it does not make sense

@@ -2203,7 +2203,12 @@ def all(self, batch_size=None):

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
:param bool flat:
Return the result as a flat list of projected entities,
with no sub-lists.

Copy link
Member

Choose a reason for hiding this comment

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

Apart from that, it looks good to me 👍

@@ -2203,7 +2203,12 @@ def all(self, batch_size=None):

Copy link
Member

Choose a reason for hiding this comment

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

Apart from that, it looks good to me 👍

This allows to simplify the commonly found form:

    results = [entry[0] for entry in builder.all()]

to the more compact and readable form:

    results = builder.all(flat=True)

The default is set to `flat=False` such that this change is fully
backwards compatible. Occurrences in the codebase itself have been
updated as much as possible.
@sphuber sphuber force-pushed the feature/3943/querybuilder-flat-result branch from 03228fe to fd296ce Compare April 22, 2020 08:10
@sphuber sphuber merged commit d6e8c7d into aiidateam:develop Apr 22, 2020
@sphuber sphuber deleted the feature/3943/querybuilder-flat-result branch April 22, 2020 08:38
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.

Add option to retrieve the results from the QueryBuilder.all() in a flat list
3 participants