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 flat argument to QueryBuilder.iterall() #5982

Open
mbercx opened this issue Apr 24, 2023 · 4 comments
Open

Add flat argument to QueryBuilder.iterall() #5982

mbercx opened this issue Apr 24, 2023 · 4 comments

Comments

@mbercx
Copy link
Member

mbercx commented Apr 24, 2023

Is your feature request related to a problem? Please describe

Typical ways to obtain the results from a query are all(), first() (for testing), and iterall(). For larger queries, the latter is usually preferable since it doesn't first put all the nodes in a list like all (I believe).

all() and first() both have the input argument flat, which flattens the list and useful to e.g. loop over a query that only has one projection per result. However, it seems iterall() doesn't have this.

Describe the solution you'd like

Let's add a flat input argument to iterall() as well, it's more consistent and useful.

@mbercx mbercx added good first issue Issues that should be relatively easy to fix also for beginning contributors topic/query-builder type/feature request status undecided labels Apr 24, 2023
@sphuber
Copy link
Contributor

sphuber commented Apr 25, 2023

I think I looked into doing this when implementing flat for all() and first() but vaguely remember that this wasn't possible or not trivial. Should maybe have a look before definitely assigning the good-first-issue label before sending someone on a wild-goose chase

@mbercx mbercx removed the good first issue Issues that should be relatively easy to fix also for beginning contributors label Apr 25, 2023
@mbercx
Copy link
Member Author

mbercx commented Apr 25, 2023

Righto! Fair point. 😅

I think I also noticed a nasty bug in iterall() last night. But have to recheck in a more clear state of mind. Will report back so if you go looking into this code we can take this into accounts.

@mbercx
Copy link
Member Author

mbercx commented Apr 25, 2023

Side note: intuitively I would expect first() to always return with flat to True. Would it be sensible to make this the default?

@sphuber
Copy link
Contributor

sphuber commented Apr 25, 2023

Side note: intuitively I would expect first() to always return with flat to True. Would it be sensible to make this the default?

Fully agree, but didn't do this because it would break backwards compatibility. Also didn't see a way for a deprecation pathway other than just warning if flat=False and say that default will become True in v3.0, but that would spam a lot of deprecation warnings by default since the default would now be False.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants