QueryBuilder: add flat keyword to first method

[sphuber]

Fixes #5408

This keyword already exists for the all method and it will likewise be
useful for first when only a single quantity is projected. In that
case, often the caller doesn't want a list as a return value but simply
the projected quantity. Allowing to get this directly from the method
call as opposed to manually dereferencing the first item from the
returned list often makes for cleaner code.

[chrisjsewell]

I still think it is important to indicate this actually executes a query, i.e. connects to the storage.

[sphuber]

Added a comment in docstring in vein of SQLA docs

[chrisjsewell]

I'd note, this is a little similar to https://docs.sqlalchemy.org/en/14/orm/query.html?highlight=scalar#sqlalchemy.orm.Query.scalar (plus thing like one and one_or_none)

I'm tempted to say this should be a separate method, because it is never real ideal when you are returning unions of different types, although actually at least add overload typing, i.e.

from typing import Any, List, Literal, overload

    @overload
    def first(self, flat: Literal[False]) -> Optional[List[Any]]:
        ...
    @overload
    def first(self, flat: Literal[True]) -> Optional[Any]:
        ...

[sphuber]

@chrisjsewell should be good for second review

[sphuber]

Are the new pre-commit failures due to the use of the overload or the from __future__ import annotations?

[chrisjsewell]

Are the new pre-commit failures due to the use of the overload or the from __future__ import annotations?

Not due to the __future__, just due to the types, i.e. surfacing "bugs" that already existed, whereby the code is assuming a list is always returned, when actually None can also be returned.
For some reason, pylint was not picking these up before.

FYI, when you are adding from __future__ import annotations, I would suggest running pyupgrade against the file, which will convert all the old style annotation to the new style: https://github.com/asottile/pyupgrade#pep-585-typing-rewrites

[sphuber]

Not due to the future, just due to the types, i.e. surfacing "bugs" that already existed, whereby the code is assuming a list is always returned, when actually None can also be returned.

But those are false positives surely? The default for flat is False, so all those calls should simply return lists and the unsubscriptable-object therefore is simply wrong. Please tell me that I don't have to start adding ignore statements for all of them just because pylint doesn't properly read the typing. In that case I see little to no benefit of adding the overloaded definitions just for the sake of the typing if all it causes us is trouble

[chrisjsewell]

But those are false positives surely? The default for flat is False, so all those calls should simply return lists

No, the return type is Optional[List[str]], i.e. a list[str] | None

[chrisjsewell]

@sphuber it can return None here

[sphuber]

Sure, I saw that, but take the first hit aiida/orm/nodes/data/upf.py:136:27:

        existing_upf = builder.first()

        if existing_upf is None:
            ...
        else:
            existing_upf = existing_upf[0]

That looks like a false positive to me. If anything this would be the perfect candidate for first(flat=True).

[sphuber]

Same goes for the next "hits"

tests/orm/test_querybuilder.py:724:26: E1136: Value 'result' is unsubscriptable (unsubscriptable-object)
tests/orm/test_querybuilder.py:725:26: E1136: Value 'result' is unsubscriptable (unsubscriptable-object)

Also false positives.

[chrisjsewell]

Fair, then IMO, I would just disable unsubscriptable-object, because pylint does not seem to be doing a good job with them: pylint-dev/pylint#1498
I don't feel pylint should be trying to involve itself in type-checking, since that is what mypy is for, that does it a lot better

[chrisjsewell]

Eurgh could also be overload, could be future annotations: pylint-dev/pylint#5189, pylint-dev/pylint#3979, pylint-dev/pylint#4369
Silly pylint

[sphuber]

@chrisjsewell pre-commit now passes. Should be good to go

[chrisjsewell]

Cheers

[chrisjsewell]

Suggested change

	def first(self, flat: bool = False) -> Optional[list[Any] | Any]:
	def first(self, flat: bool = False) -> None | list[Any] | Any:

Will approve anyway, but you are kind of mixing old and new type annotations in these changes