Fix little problem with type hints and adopt a test to make sure it works in all platform #1207
Conversation
|
@Zoupers Is this ready for review? And can you specify in the description or inline, which test is the one that didn't work on Windows? |
src/flask_sqlalchemy/query.py
Outdated
| from flask import abort | ||
|
|
||
| from .pagination import Pagination | ||
| from .pagination import QueryPagination | ||
|
|
||
|
|
||
| class Query(sa.orm.Query): # type: ignore[type-arg] | ||
| class Query(sa_orm.Query[t.Any]): |
There was a problem hiding this comment.
There was a problem hiding this comment.
According to this, https://github.com/sqlalchemy/sqlalchemy/blob/4808acfe1b88854fce47b792ef9995f1921426e7/lib/sqlalchemy/orm/query.py#L77
which is the version(1.4.18) specified in the requirements/tests-min.in, it's not correct. I will fix it.
| @@ -24,5 +26,5 @@ class Example(db.Model): | |||
| assert "FROM example" in info.statement | |||
| assert info.parameters[0][0] == 5 | |||
| assert info.duration == info.end_time - info.start_time | |||
| assert "tests/test_record_queries.py:" in info.location | |||
| assert os.path.join("tests", "test_record_queries.py:") in info.location | |||
There was a problem hiding this comment.
I assume this is the test that did not previously work in Windows.
|
I have fixed the bug and passed the corresponding tests locally. I apologize for any inconvenience caused. |
|
@Zoupers Thanks! I think we may upgrade min version of SQLAlchemy to 2.0 in the next version in order to formally support it, but for this minor release, it's better to just get it working for 1.4. This is looking good now. |
|
I experimented a bit to see if we could automatically test that this works correctly, but we're not currently running pyright on the code, and if we did, we'd have a bunch of other issues that can't be ignored as easily as they are in mypy (as pyright doesn't have the same specific error codes), so I think it's fine to not have a test for the pyright-specific typing right now. However, if there's a regression in the future, we might consider adding pyright env to tox. |
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 inCHANGES.rstsummarizing the change and linking to the issue.Add.. versionchanged::entries in any relevant code docs.pre-commithooks and fix any issues.pytestandtox, no tests failed.Delete line means I think there is no need to do that.