👌 IMPROVE: `QueryBuilder` reprs (incl/ queryhelp -> as_dict) #5081

chrisjsewell · 2021-08-15T13:02:28Z

This PR principally does:

Deprecates queryhelp for as_dict() and also add a from_dict classmethod

As I believe @mbercx and @CasperWA already agree, queryhelp is a very unintuitive name, and a stumbling block for many new users.

Replace the __str__ outputs with a representation of as_dict() rather than the compiled SQL query (and also use this for __repr__).

Users should not need to be exposed to the SQL, this is a "dev-user" feature

Add an as_sql method, for retrieving the SQL query string

This has an option to allow for both strings with the parameters inlined or external.
It also fixes #3844 and #4659 (since the SQL string is no longer generated on print, this is also less of an issue)

Add analyze_query method, to run postgresql's EXPLAIN command.
Some more type hinting and tests

closes #4502
closes #3844
closes #3845
closes #4659

…iida_core into querybuilder-repr

for more information, see https://pre-commit.ci

codecov · 2021-08-16T00:43:48Z

Codecov Report

Merging #5081 (06c1e60) into develop (74571a1) will increase coverage by 0.06%.
The diff coverage is 89.39%.

@@             Coverage Diff             @@
##           develop    #5081      +/-   ##
===========================================
+ Coverage    80.44%   80.50%   +0.06%     
===========================================
  Files          531      531              
  Lines        36973    37020      +47     
===========================================
+ Hits         29738    29798      +60     
+ Misses        7235     7222      -13

Flag	Coverage Δ
django	`74.99% <86.73%> (+0.07%)`	⬆️
sqlalchemy	`73.93% <84.08%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/tools/importexport/dbexport/main.py	`97.31% <ø> (ø)`
aiida/orm/implementation/django/querybuilder.py	`78.75% <75.00%> (+0.91%)`	⬆️
...iida/orm/implementation/sqlalchemy/querybuilder.py	`82.03% <87.50%> (+0.99%)`	⬆️
aiida/orm/querybuilder.py	`83.94% <90.13%> (+2.19%)`	⬆️
aiida/tools/graph/age_rules.py	`89.19% <93.34%> (ø)`
aiida/restapi/resources.py	`97.26% <100.00%> (ø)`
aiida/transports/plugins/local.py	`81.41% <0.00%> (-0.25%)`	⬇️

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 74571a1...06c1e60. Read the comment docs.

chrisjsewell · 2021-08-16T09:01:52Z

Ok this is done, I would like to merge this asap, to feed it up into #5069. So looking to merge in next day or two

chrisjsewell · 2021-08-16T09:05:17Z

aiida/orm/querybuilder.py

+    @property
+    def queryhelp(self) -> QueryDict:
+        """"Legacy name for ``as_dict`` method."""
+        # deprecate?


The only outstanding question, for me, is whether we actually add a deprecation warning here, or "schedule" it for a later date. I guess essentially it shouldn't be removed until v3?

I don't think it will be good for users to have deorecation warnings showing up when they've just switched to v2, so I would be tempted to leave it for now

CasperWA

Cool, thanks @chrisjsewell !

Some comments along with the actual review comments:
Maybe rename the awkward _kw to the more standard kwargs?
Are the *.txt and test_as_dict.yml files in the test folder for the QueryBuilder necessary? They seem to maybe be relics from some tests that should be removed or are they intentional?

aiida/orm/querybuilder.py

aiida/restapi/resources.py

docs/source/topics/database.rst

chrisjsewell · 2021-08-16T09:50:22Z

cheers for the quick review @CasperWA 😄

chrisjsewell · 2021-08-16T09:52:03Z

Are the *.txt and test_as_dict.yml files in the test folder for the QueryBuilder necessary? They seem to maybe be relics from some tests that should be removed or are they intentional?

No they are intentional and generated in tests/orm/test_querybuilder.py::TestRepresentations by pytest-regressions

chrisjsewell · 2021-08-16T09:55:51Z

Maybe rename the awkward _kw to the more standard kwargs?

done

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

aiida/orm/querybuilder.py

CasperWA · 2021-08-16T10:41:50Z

No they are intentional and generated in tests/orm/test_querybuilder.py::TestRepresentations by pytest-regressions

Why? Is there a reason this is necessary to keep in the repo? Couldn't it just be deleted after use or kept in memory if it's to be used elsewhere?

chrisjsewell · 2021-08-16T11:06:14Z

No they are intentional and generated in tests/orm/test_querybuilder.py::TestRepresentations by pytest-regressions

Why? Is there a reason this is necessary to keep in the repo? Couldn't it just be deleted after use or kept in memory if it's to be used elsewhere?

No this is just how https://pytest-regressions.readthedocs.io works; it’s regressing against the stored files. I’d definitely recommend it, we use it loads, for example, in aiida-quantumespresso

aiida/orm/querybuilder.py

chrisjsewell · 2021-08-16T16:45:16Z

@CasperWA I've addressed all your comments cheers

chrisjsewell · 2021-08-17T08:03:19Z

merging this today (the link check fail is just that https://shapeshed.com/unix-exit-codes/ is down at present)

CasperWA · 2021-08-17T08:31:06Z

merging this today (the link check fail is just that https://shapeshed.com/unix-exit-codes/ is down at present)

Sure - but maybe always wait for an approving review, or?

CasperWA

Thanks a bunch @chrisjsewell !
Approved on my end. I found a single "one sentence per one line" thing in the documentation and that's about it.

docs/source/topics/database.rst

chrisjsewell · 2021-08-17T08:39:41Z

but maybe always wait for an approving review,

I had full faith that you would sign it off today 😄

chrisjsewell · 2021-08-17T08:40:57Z

Approved on my end

Thanks!

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

chrisjsewell added 2 commits August 15, 2021 11:30

👌 IMPROVE: QueryBuilder representations

c696267

remove queryhelp

0636376

chrisjsewell requested review from ltalirz, sphuber, mbercx and giovannipizzi August 15, 2021 13:02

chrisjsewell and others added 5 commits August 15, 2021 15:06

Merge branch 'develop' into querybuilder-repr

5e9dfaf

fix test

b573232

Merge branch 'querybuilder-repr' of https://github.com/chrisjsewell/a…

a70fa63

…iida_core into querybuilder-repr

fixes

dceee09

[pre-commit.ci] auto fixes from pre-commit.com hooks

9a6fb1b

for more information, see https://pre-commit.ci

chrisjsewell added 2 commits August 16, 2021 03:55

add analyze_query

7cc5870

fix literal rendering

4b616d6

chrisjsewell commented Aug 16, 2021

View reviewed changes

fix pre-commit

37b6f64

CasperWA suggested changes Aug 16, 2021

View reviewed changes

_kw -> kwargs

f1f9c36

Apply suggestions from code review

e1da060

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

chrisjsewell commented Aug 16, 2021

View reviewed changes

aiida/orm/querybuilder.py Outdated Show resolved Hide resolved

Update aiida/orm/querybuilder.py

f257372

chrisjsewell commented Aug 16, 2021

View reviewed changes

aiida/orm/querybuilder.py Show resolved Hide resolved

chrisjsewell added 2 commits August 16, 2021 13:57

Update aiida/orm/querybuilder.py

49bd772

add __repr__

5636329

ltalirz removed their request for review August 16, 2021 12:58

minor update

164d17a

chrisjsewell mentioned this pull request Aug 16, 2021

✨ NEW: Add querybuilder jsonschema #5083

Closed

minor fixes

afe7ab1

chrisjsewell requested a review from CasperWA August 16, 2021 16:45

chrisjsewell changed the title ~~👌 IMPROVE: QueryBuilder representations (replace queryhelp)~~ 👌 IMPROVE: QueryBuilder reprs (incl/ queryhelp -> as_dict) Aug 16, 2021

Use typing_extensions.TypedDict for python 3.7

2ef4177

CasperWA previously approved these changes Aug 17, 2021

View reviewed changes

docs/source/topics/database.rst Outdated Show resolved Hide resolved

Update docs/source/topics/database.rst

06c1e60

Co-authored-by: Casper Welzel Andersen <43357585+CasperWA@users.noreply.github.com>

chrisjsewell dismissed CasperWA’s stale review via 06c1e60 August 17, 2021 08:41

chrisjsewell merged commit ccd5795 into aiidateam:develop Aug 17, 2021

chrisjsewell deleted the querybuilder-repr branch August 17, 2021 09:39

This was referenced Aug 25, 2021

♻️ REFACTOR: Fully abstract QueryBuilder #5093

Merged

Deprecate queryhelp #5096

Closed

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

👌 IMPROVE: `QueryBuilder` reprs (incl/ queryhelp -> as_dict) #5081

👌 IMPROVE: `QueryBuilder` reprs (incl/ queryhelp -> as_dict) #5081

chrisjsewell commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 16, 2021 •

edited

Loading

chrisjsewell commented Aug 16, 2021

chrisjsewell Aug 16, 2021

chrisjsewell Aug 16, 2021 •

edited

Loading

CasperWA left a comment

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

CasperWA commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 17, 2021 •

edited

Loading

CasperWA commented Aug 17, 2021

CasperWA left a comment

chrisjsewell commented Aug 17, 2021

chrisjsewell commented Aug 17, 2021

👌 IMPROVE: QueryBuilder reprs (incl/ queryhelp -> as_dict) #5081

👌 IMPROVE: QueryBuilder reprs (incl/ queryhelp -> as_dict) #5081

Conversation

chrisjsewell commented Aug 15, 2021 • edited Loading

codecov bot commented Aug 16, 2021 • edited Loading

Codecov Report

chrisjsewell commented Aug 16, 2021

chrisjsewell Aug 16, 2021

Choose a reason for hiding this comment

chrisjsewell Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

CasperWA left a comment

Choose a reason for hiding this comment

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

CasperWA commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 16, 2021

chrisjsewell commented Aug 17, 2021 • edited Loading

CasperWA commented Aug 17, 2021

CasperWA left a comment

Choose a reason for hiding this comment

chrisjsewell commented Aug 17, 2021

chrisjsewell commented Aug 17, 2021

👌 IMPROVE: `QueryBuilder` reprs (incl/ queryhelp -> as_dict) #5081

👌 IMPROVE: `QueryBuilder` reprs (incl/ queryhelp -> as_dict) #5081

chrisjsewell commented Aug 15, 2021 •

edited

Loading

codecov bot commented Aug 16, 2021 •

edited

Loading

chrisjsewell Aug 16, 2021 •

edited

Loading

chrisjsewell commented Aug 17, 2021 •

edited

Loading