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

Support for sqlalchemy 1.4 or optionally disable specific stubs #845

Closed
michaeloliverx opened this issue Jan 17, 2021 · 8 comments
Closed
Labels
needs decision Do we want this enhancement? typestub Issue relating to our bundled type stubs

Comments

@michaeloliverx
Copy link

Hi, firstly thanks for pylance it really is amazing!

I am currently using a pre-release version of SQLAlchemy 1.4. It includes some changes to the mapper configuration:

from sqlalchemy import Column, Integer, String, Text, ForeignKey

from sqlalchemy.orm import registry
from sqlalchemy.orm import relationship

mapper_registry = registry()

@mapper_registry.mapped
class User:
    __tablename__ = 'user'

    id = Column(Integer, primary_key=True)
    name = Column(String)

    addresses = relationship("Address", back_populates="user")

@mapper_registry.mapped
class Address:
    __tablename__ = 'address'

    id = Column(Integer, primary_key=True)
    user_id = Column(ForeignKey("user.id"))
    email_address = Column(String)

    user = relationship("User", back_populates="addresses")

The bundled stub pylance includes for SQLAlchemy are not compatible with 1.4+:
image

If you are using PEP 484 static type checkers for Python, a MyPy plugin is included with type stubs for SQLAlchemy. The plugin is tailored towards SQLAlchemy declarative models. SQLAlchemy hopes to include more comprehensive PEP 484 support in future releases.
Source

Since 1.4 includes some type stubs for mypy it may cause some other clashes in the future.

Is it possible to get some minor support of this version or at least some version detection and enable/disable based on that?

#837 discusses disabling the bundled SQLAlchemy stubs, I think having the ability to enable/disable specific bundled type stubs would be a great.

References

@judej judej added the needs investigation Could be an issue - needs investigation label Jan 19, 2021
@github-actions github-actions bot removed the triage label Jan 19, 2021
@jakebailey
Copy link
Member

This surprises me, as we should be getting types from SQLAlchemy directly if they are provided along side it. The bundled stubs are the last place we look and should not be taking precedence. As a workaround, you could delete the stubs from the Pylance bundle and see if that helps.

Looking at that source link, though, it doesn't seem to say that stubs are included, only that you can install those stubs to get some amount of support, no? We aren't mypy, so its plugin doesn't really do anything for us. Constructs that can't be defined in terms of type annotations and instead rely on per-type-checker plugins are really unfortunate...

@smithed180
Copy link

I can confirm I just installed https://pypi.org/project/sqlalchemy2-stubs/ (and then uninstalled mypy), this seems to work with sqla 1.4 including the new "future=true" param and the moving of the location of declarative_base between files.

It does not seem to handle hybrid_property decorators, I'm not sure if thats a pylance bug or a type stub bug. With a regular property decorator pylance accepts two functions with the same name for getter and setter. This is not handled with hybrid_property.

@erictraut
Copy link
Contributor

Pyright (the type checker upon which pylance is based) contains a bunch of special logic for the @Property class since it has very special semantics and is a common stdlib construct. We don't include special logic for third-party libraries that have unusual or non-standard semantics. So unfortunately, the hybrid_property for sqlalchemy won't work the way you hope it does.

@jakebailey
Copy link
Member

Thanks for linking to that package (https://github.com/sqlalchemy/sqlalchemy2-stubs). They seem like they're in alpha, but we will probably want to switch to these in our bundle at some point given they support 1.4 and 2 and are actually in the sqlalchemy org on GitHub.

@smithed180
Copy link

Yeah, definitely in alpha. The sqlalchemy2-stubs are back to having no idea what the return is for a query. In other words, session.query() returns unknown, so then .all() and .one() don't work and the final return is also unknown. This is enough of an annoyance I'm uninstalling it, but its at least worth knowing about. Might look back in more detail later but for right now I can put up with a few bad imports and function signatures in exchange for not having to manually type every query result :)

@mg-christian-axelsson
Copy link

mg-christian-axelsson commented Aug 4, 2021

I tried the sqlalchemy2-stubs package today due to model column type issues (as described in #837) and unfortunately the session.query() (and all other common methods on session) issue still persists when session is a scoped_session.
I believe them fixing sqlalchemy/sqlalchemy2-stubs#61 will solve that issue.

@zzzeek
Copy link

zzzeek commented Dec 30, 2021

is this a dupe of #840? or is the issue here that of automatically determining the right stubs for SQLAlchemy specifically without any user intervention ?

@heejaechang heejaechang self-assigned this Apr 13, 2022
@heejaechang heejaechang added typestub Issue relating to our bundled type stubs needs decision Do we want this enhancement? and removed needs investigation Could be an issue - needs investigation labels Apr 19, 2022
@judej
Copy link
Contributor

judej commented Apr 19, 2022

SQLAlchemy stub issue. That is outside what we can do

@judej judej closed this as completed Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision Do we want this enhancement? typestub Issue relating to our bundled type stubs
Projects
None yet
Development

No branches or pull requests

8 participants