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 type to get_or_404, update tests #1208

Merged
merged 7 commits into from
Jun 19, 2023

Conversation

pamelafox
Copy link
Contributor

@pamelafox pamelafox commented May 23, 2023

This PR fixes a typing issue brought up by @jdimmerman in this comment:
#1140 (comment)

Specifically:

todo1 = db.session.get(Todo, my_id)  # properly typed as Todo | None
todo2 = db.get_or_404(Todo, my_id)  # improperly typed as Any
todo3 = db.session.query(Todo).all()  # properly typed as List[Todo]
todo4 = Todo.query.all()  # improperly typed as Any

While making/testing that change, I realized that the only tests of the *_or_404 functions are in test_legacy_query.py, despite them also being available with the non-legacy API. So I copied them into a new test file and adjusted them to use the current API. That does mean there's some redundancy in the tests, but also makes it easy to delete the legacy tests at some point if that becomes fully deprecated.

In terms of testing this change, I added a test that demonstrates the typing and passes pdm run mypy, but that test also pass mypy before, since the typing has gone from t.Any to t.Optional[ModelRef].

Python 3.11 has an explicit typing.assert_type function for use by type checkers, so I modified tox.ini to run mypy for both 3.7 and 3.11, and put assert_type behind a hasattr guard in the test. That fails before the change and succeeds after!

Here's what it looks like in VS Code:

Screenshot 2023-05-23 at 10 47 16 AM

Checklist:

  • Add tests that demonstrate the correct behavior of the change.
  • Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

(I struck out tasks that don't seem warranted for a typing change)

@@ -730,8 +732,8 @@ def get_binds(self) -> dict[sa.Table, sa.engine.Engine]:
}

def get_or_404(
self, entity: type[t.Any], ident: t.Any, *, description: str | None = None
) -> t.Any:
self, entity: type[_O], ident: t.Any, *, description: str | None = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change based on how it's done in SQLAlchemy for session.get():

    def get(
        self,
        entity: _EntityBindKey[_O],
        ident: _PKIdentityArgument,
        *,
        options: Optional[Sequence[ORMOption]] = None,
        populate_existing: bool = False,
        with_for_update: ForUpdateParameter = None,
        identity_token: Optional[Any] = None,
        execution_options: OrmExecuteOptionsParameter = util.EMPTY_DICT,
        bind_arguments: Optional[_BindArguments] = None,
    ) -> Optional[_O]:

I left off the _EntityBindKey, as that didn't seem necessary for the expected developer experience.

.gitignore Outdated Show resolved Hide resolved
@@ -23,6 +23,8 @@
from .session import Session
from .table import _Table

_O = t.TypeVar("_O", bound=object)

Choose a reason for hiding this comment

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

What does bound=object do for this variable? The original type was type[t.Any] so effectively boundless.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I'm attempting to copy SQLAlchemy, but now I realize that they bind to Any for this case:
https://github.com/sqlalchemy/sqlalchemy/blob/2dced78f33010d4a6a6276f4e8c5665bd08d01c0/lib/sqlalchemy/orm/_typing.py#L63

There are other cases where they bind to object (_OO) but it's unclear why they make that distinction. I asked in the python discussion forum about the differences:
https://discuss.python.org/t/differences-in-bound-object-vs-bound-any/27052/2
I may ask in the SQLAlchemy forum as well.

I'm updating this diff now to bind to t.Any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And now, thanks to this discussion and the related threads, sqlalchemy updated the Any to object, so I've changed it back to object. Bit more bounded now!

@pamelafox pamelafox requested a review from tonybaloney June 10, 2023 17:56
@tonybaloney
Copy link

LGTM 👍

@pamelafox
Copy link
Contributor Author

Update: Figured out how to use assert_type in 3.11 with mypy to test this change.

@pamelafox pamelafox merged commit 4d1cc1a into pallets-eco:3.0.x Jun 19, 2023
@pamelafox pamelafox deleted the get-or-404-type branch June 19, 2023 16:56
@jdimmerman
Copy link
Contributor

jdimmerman commented Jun 21, 2023

@pamelafox amazing - thank you for doing this!

One question - shouldn't the return type be <Model> rather than <Model> | None (wherein <Model> is the model class type) given than the request will be aborted if a row is not returned?
(Meaning the return type should be _O instead of t.Optional[_O])

@pamelafox
Copy link
Contributor Author

@jdimmerman Right you are! I discovered the issue once I merged 3.0.4 into main, as a new test there happened to fail mypy check.
PR with the fix is here: #1226
I'll release 3.0.5 today with that and another (unrelated) fix.

@jdimmerman
Copy link
Contributor

@pamelafox once step ahead - thanks!!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants