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.

Sign up for GitHub

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

Jump to bottom

(Bad rebase) Fix issue #1318: Type (typehint) error for db.relationship #1319

Closed
wants to merge 20 commits into from
Closed

(Bad rebase) Fix issue #1318: Type (typehint) error for db.relationship #1319

wants to merge 20 commits into from

Conversation

cainmagi
Copy link

Fix the typehint inconsistence of db.relationship(...).

Checklist:

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

@cainmagi cainmagi changed the title Fix issue 1318 Fix issue #1318: Type (typehint) error for db.relationship Mar 26, 2024
pamelafox
pamelafox reviewed Mar 26, 2024
View reviewed changes
CHANGES.rst Outdated

- Fix the type check failure caused by inconsistency between
`SQLAlchemy().relationship` and `sqlalchemy.orm.relationship`. :issue:`1318`
- Add `allowlist_externals` property to `tox.ini` to prevent the failure that `mypy`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a contributors-only change, right? I don't think we'd typically put that in the CHANGES. @davidism can correct me if I'm wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, from an end user point of view i would not care about such things...

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for letting me know! I will remove this part after I rebase this PR.

pamelafox
pamelafox reviewed Mar 26, 2024
View reviewed changes
src/flask_sqlalchemy/extension.py
def relationship(
self, *args: t.Any, **kwargs: t.Any
) -> sa_orm.RelationshipProperty[t.Any]:
def relationship(self, *args: t.Any, **kwargs: t.Any) -> sa_orm.Relationship[t.Any]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any tests that use relationship()? I've been able to get test failures in the past for typing errors, since mypy runs on the tests.

Copy link
Author

Choose a reason for hiding this comment

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

Hello, @pamelafox ! Could you let me know what kind of test you are referring to? I have run pytest and tox p. Both tests passed.

I noticed there are tests like these:

class Child(db.Model):
        id = sa.Column(sa.Integer, primary_key=True)
        parent_id = sa.Column(sa.ForeignKey(Parent.id))  # type: ignore[var-annotated]
        parent2 = db.relationship(
            Parent,
            backref=db.backref("children2", lazy="dynamic", viewonly=True),
            viewonly=True,
        )

    p = Parent()
    assert type(Parent.query) is Query
    assert isinstance(p.children1, Query)
    # Cannot access member "children2" for type "Parent"
    # Member "children2" is unknownPylance[reportAttributeAccessIssue]
    assert isinstance(p.children2, Query)  
    assert isinstance(db.session.query(Child), Query)

But the type checking issues like this is not caused by wrong typehints. That's because the dynamically created backref cannot be detected by static type checkers. I think this issue is not related to #1318

@pamelafox
Copy link
Contributor

It looks like you branched off main, but bugfixes should branch off latest 3.x branch, per
https://github.com/pallets-eco/flask-sqlalchemy/blob/main/CONTRIBUTING.rst#start-coding

Can you rebase to branch off that branch instead? Thanks!

@cainmagi
Copy link
Author

Sorry for not noticing it! I will try to rebase this PR later.

dependabot bot and others added 17 commits March 26, 2024 14:36
@dependabot @cainmagi
Bump the github-actions group with 3 updates
fda01e2 
Bumps the github-actions group with 3 updates: [actions/checkout](https://github.com/actions/checkout), [actions/upload-artifact](https://github.com/actions/upload-artifact) and [actions/cache](https://github.com/actions/cache).


Updates `actions/checkout` from 3.6.0 to 4.0.0
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@f43a0e5...3df4ab1)

Updates `actions/upload-artifact` from 3.1.2 to 3.1.3
- [Release notes](https://github.com/actions/upload-artifact/releases)
- [Commits](actions/upload-artifact@0b7f8ab...a8a3f3a)

Updates `actions/cache` from 3.3.1 to 3.3.2
- [Release notes](https://github.com/actions/cache/releases)
- [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md)
- [Commits](actions/cache@88522ab...704facf)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: actions/upload-artifact
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: actions/cache
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @cainmagi
Bump the github-actions group with 1 update
0925777 
Bumps the github-actions group with 1 update: [actions/checkout](https://github.com/actions/checkout).

- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@3df4ab1...8ade135)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @cainmagi
Bump the python-requirements group in /requirements with 2 updates
6a52dec 
Bumps the python-requirements group in /requirements with 2 updates: [sphinx](https://github.com/sphinx-doc/sphinx) and [sqlalchemy](https://github.com/sqlalchemy/sqlalchemy).


Updates `sphinx` from 7.2.5 to 7.2.6
- [Release notes](https://github.com/sphinx-doc/sphinx/releases)
- [Changelog](https://github.com/sphinx-doc/sphinx/blob/master/CHANGES.rst)
- [Commits](sphinx-doc/sphinx@v7.2.5...v7.2.6)

Updates `sqlalchemy` from 2.0.20 to 2.0.21
- [Release notes](https://github.com/sqlalchemy/sqlalchemy/releases)
- [Changelog](https://github.com/sqlalchemy/sqlalchemy/blob/main/CHANGES.rst)
- [Commits](https://github.com/sqlalchemy/sqlalchemy/commits)

---
updated-dependencies:
- dependency-name: sphinx
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: sqlalchemy
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
...

Signed-off-by: dependabot[bot] <[email protected]>
@pre-commit-ci @cainmagi
[pre-commit.ci] pre-commit autoupdate (pallets-eco#1265)
b71eec9 
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@dependabot @cainmagi
Bump urllib3 from 2.0.4 to 2.0.6 in /requirements (pallets-eco#1264)
7d7ba77 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @cainmagi
Bump urllib3 from 2.0.6 to 2.0.7 in /requirements (pallets-eco#1270)
55a8c0b 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @cainmagi
Bump the github-actions group with 2 updates
9556d68 
Bumps the github-actions group with 2 updates: [actions/checkout](https://github.com/actions/checkout) and [actions/setup-python](https://github.com/actions/setup-python).


Updates `actions/checkout` from 4.1.0 to 4.1.1
- [Release notes](https://github.com/actions/checkout/releases)
- [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
- [Commits](actions/checkout@8ade135...b4ffde6)

Updates `actions/setup-python` from 4.7.0 to 4.7.1
- [Release notes](https://github.com/actions/setup-python/releases)
- [Commits](actions/setup-python@61a6322...65d7f2d)

---
updated-dependencies:
- dependency-name: actions/checkout
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
- dependency-name: actions/setup-python
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @cainmagi
Bump the python-requirements group in /requirements with 6 updates
1413acc 
Bumps the python-requirements group in /requirements with 6 updates:

| Package | From | To |
| --- | --- | --- |
| [blinker](https://github.com/pallets-eco/blinker) | `1.6.2` | `1.6.3` |
| [coverage[toml]](https://github.com/nedbat/coveragepy) | `7.3.1` | `7.3.2` |
| [mypy](https://github.com/python/mypy) | `1.5.1` | `1.6.1` |
| [pre-commit](https://github.com/pre-commit/pre-commit) | `3.4.0` | `3.5.0` |
| [pytest](https://github.com/pytest-dev/pytest) | `7.4.2` | `7.4.3` |
| [sqlalchemy](https://github.com/sqlalchemy/sqlalchemy) | `2.0.21` | `2.0.22` |


Updates `blinker` from 1.6.2 to 1.6.3
- [Release notes](https://github.com/pallets-eco/blinker/releases)
- [Changelog](https://github.com/pallets-eco/blinker/blob/main/CHANGES.rst)
- [Commits](pallets-eco/blinker@1.6.2...1.6.3)

Updates `coverage[toml]` from 7.3.1 to 7.3.2
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.3.1...7.3.2)

Updates `mypy` from 1.5.1 to 1.6.1
- [Changelog](https://github.com/python/mypy/blob/master/CHANGELOG.md)
- [Commits](python/mypy@v1.5.1...v1.6.1)

Updates `pre-commit` from 3.4.0 to 3.5.0
- [Release notes](https://github.com/pre-commit/pre-commit/releases)
- [Changelog](https://github.com/pre-commit/pre-commit/blob/main/CHANGELOG.md)
- [Commits](pre-commit/pre-commit@v3.4.0...v3.5.0)

Updates `pytest` from 7.4.2 to 7.4.3
- [Release notes](https://github.com/pytest-dev/pytest/releases)
- [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst)
- [Commits](pytest-dev/pytest@7.4.2...7.4.3)

Updates `sqlalchemy` from 2.0.21 to 2.0.22
- [Release notes](https://github.com/sqlalchemy/sqlalchemy/releases)
- [Changelog](https://github.com/sqlalchemy/sqlalchemy/blob/main/CHANGES.rst)
- [Commits](https://github.com/sqlalchemy/sqlalchemy/commits)

---
updated-dependencies:
- dependency-name: blinker
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: coverage[toml]
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: mypy
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: python-requirements
- dependency-name: pre-commit
  dependency-type: direct:development
  update-type: version-update:semver-minor
  dependency-group: python-requirements
- dependency-name: pytest
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: sqlalchemy
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
...

Signed-off-by: dependabot[bot] <[email protected]>
@heavelock @cainmagi
Fix PR id in the changelog
6fb263c
@dependabot @cainmagi
Bump the github-actions group with 2 updates
71333de 
Bumps the github-actions group with 2 updates: [dessant/lock-threads](https://github.com/dessant/lock-threads) and [pypa/gh-action-pypi-publish](https://github.com/pypa/gh-action-pypi-publish).


Updates `dessant/lock-threads` from 4.0.1 to 5.0.1
- [Release notes](https://github.com/dessant/lock-threads/releases)
- [Changelog](https://github.com/dessant/lock-threads/blob/main/CHANGELOG.md)
- [Commits](dessant/lock-threads@be8aa5b...1bf7ec2)

Updates `pypa/gh-action-pypi-publish` from 1.8.10 to 1.8.11
- [Release notes](https://github.com/pypa/gh-action-pypi-publish/releases)
- [Commits](pypa/gh-action-pypi-publish@b7f401d...2f6f737)

---
updated-dependencies:
- dependency-name: dessant/lock-threads
  dependency-type: direct:production
  update-type: version-update:semver-major
  dependency-group: github-actions
- dependency-name: pypa/gh-action-pypi-publish
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: github-actions
...

Signed-off-by: dependabot[bot] <[email protected]>
@dependabot @cainmagi
Bump the python-requirements group in /requirements with 5 updates
5c982b4 
Bumps the python-requirements group in /requirements with 5 updates:

| Package | From | To |
| --- | --- | --- |
| [blinker](https://github.com/pallets-eco/blinker) | `1.6.3` | `1.7.0` |
| [coverage[toml]](https://github.com/nedbat/coveragepy) | `7.3.1` | `7.3.2` |
| [mypy](https://github.com/python/mypy) | `1.6.1` | `1.7.1` |
| [sqlalchemy](https://github.com/sqlalchemy/sqlalchemy) | `2.0.22` | `2.0.23` |
| [tox](https://github.com/tox-dev/tox) | `4.11.3` | `4.11.4` |


Updates `blinker` from 1.6.3 to 1.7.0
- [Release notes](https://github.com/pallets-eco/blinker/releases)
- [Changelog](https://github.com/pallets-eco/blinker/blob/main/CHANGES.rst)
- [Commits](pallets-eco/blinker@1.6.3...1.7.0)

Updates `coverage[toml]` from 7.3.1 to 7.3.2
- [Release notes](https://github.com/nedbat/coveragepy/releases)
- [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst)
- [Commits](nedbat/coveragepy@7.3.1...7.3.2)

Updates `mypy` from 1.6.1 to 1.7.1
- [Changelog](https://github.com/python/mypy/blob/master/CHANGELOG.md)
- [Commits](python/mypy@v1.6.1...v1.7.1)

Updates `sqlalchemy` from 2.0.22 to 2.0.23
- [Release notes](https://github.com/sqlalchemy/sqlalchemy/releases)
- [Changelog](https://github.com/sqlalchemy/sqlalchemy/blob/main/CHANGES.rst)
- [Commits](https://github.com/sqlalchemy/sqlalchemy/commits)

Updates `tox` from 4.11.3 to 4.11.4
- [Release notes](https://github.com/tox-dev/tox/releases)
- [Changelog](https://github.com/tox-dev/tox/blob/main/docs/changelog.rst)
- [Commits](tox-dev/tox@4.11.3...4.11.4)

---
updated-dependencies:
- dependency-name: blinker
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: python-requirements
- dependency-name: coverage[toml]
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: mypy
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: python-requirements
- dependency-name: sqlalchemy
  dependency-type: direct:production
  update-type: version-update:semver-patch
  dependency-group: python-requirements
- dependency-name: tox
  dependency-type: direct:development
  update-type: version-update:semver-patch
  dependency-group: python-requirements
...

Signed-off-by: dependabot[bot] <[email protected]>
@pre-commit-ci @cainmagi
[pre-commit.ci] pre-commit autoupdate
eb838fe 
updates:
- [github.com/asottile/pyupgrade: v3.14.0 → v3.15.0](asottile/pyupgrade@v3.14.0...v3.15.0)
- [github.com/psf/black: 23.9.1 → 23.11.0](psf/black@23.9.1...23.11.0)
- [github.com/pre-commit/pre-commit-hooks: v4.4.0 → v4.5.0](pre-commit/pre-commit-hooks@v4.4.0...v4.5.0)
@pre-commit-ci @pamelafox @cainmagi
[pre-commit.ci] pre-commit autoupdate (pallets-eco#1300)
0fa0f07 
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Pamela Fox <[email protected]>
@dependabot @pamelafox @cainmagi
Bump jinja2 from 3.1.2 to 3.1.3 in /requirements (pallets-eco#1304)
ef0109f 
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pamela Fox <[email protected]>
@dependabot @cainmagi
Bump the python-requirements group in /requirements with 6 updates (p…
9d0a065 
…allets-eco#1305)

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@cainmagi
Fix issue pallets-eco#1318
ac14a38 
Fix the typehint inconsistence of `db.relationship(...)`.
@cainmagi
Fix an issue when tox p fails because mypy is forbidden.
5012474 
See details here: https://stackoverflow.com/questions/47642747/tox-warningtest-command-found-but-not-installed-in-testenv
@cainmagi
Copy link
Author

Sorry! It seems that the branch is messed up after rebasing it! I think I should submit a different PR to do this.

@cainmagi cainmagi closed this Mar 26, 2024
@cainmagi cainmagi changed the title Fix issue #1318: Type (typehint) error for db.relationship (Bad rebase) ~Fix issue #1318: Type (typehint) error for db.relationship~ Mar 26, 2024
@cainmagi cainmagi changed the title (Bad rebase) ~Fix issue #1318: Type (typehint) error for db.relationship~ (Bad rebase) ~~Fix issue #1318: Type (typehint) error for db.relationship~~ Mar 26, 2024
@cainmagi cainmagi changed the title (Bad rebase) ~~Fix issue #1318: Type (typehint) error for db.relationship~~ (Bad rebase) Fix issue #1318: Type (typehint) error for db.relationship Mar 26, 2024
@cainmagi cainmagi changed the title (Bad rebase) Fix issue #1318: Type (typehint) error for db.relationship (Bad rebase) ~~Fix issue #1318: Type (typehint) error for db.relationship~~ Mar 26, 2024
@cainmagi cainmagi changed the title (Bad rebase) ~~Fix issue #1318: Type (typehint) error for db.relationship~~ (Bad rebase) Fix issue #1318: Type (typehint) error for db.relationship Mar 26, 2024
@cainmagi cainmagi deleted the fix-issue-1318 branch March 26, 2024 19:53
@cainmagi cainmagi mentioned this pull request Mar 26, 2024
Open
6 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pamelafox pamelafox pamelafox left review comments

@ThiefMaster ThiefMaster ThiefMaster left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type (typehint) error for db.relationship
5 participants
@cainmagi @pamelafox @ThiefMaster @yuxiaoy1 @heavelock