sqlalchemy / sqlalchemy2-stubs

PEP-484 typing stubs for SQLAlchemy 1.4
MIT License
157 stars 41 forks source link

stubs use implicit reexports #187

Open rectalogic opened 2 years ago

rectalogic commented 2 years ago

Describe the bug mypy enables --no-implicit-reexport for stub files:

Note this is always treated as enabled for stub files.

https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-no-implicit-reexport Many of the sqlalchemy stubs reexport and so mypy seems to not recognize those exports.

Expected behavior mypy should not fail

To Reproduce cc.py:

import sqlalchemy

CC = sqlalchemy.sql.expression.ColumnClause

Error

$ mypy cc.py
cc.py:3: error: Module has no attribute "ColumnClause"
Found 1 error in 1 file (checked 1 source file)

Versions.

Additional context Manually editing .../lib/python3.8/site-packages/sqlalchemy-stubs/sql/expression.pyi and changing the import of ColumnClause to from .elements import ColumnClause as ColumnClause fixes this.

There are other objects that are also implicitly re-exported and result in mypy errors - sqlalchemy.sql.expression.Executable, sqlalchemy.sql.expression.FunctionElement, sqlalchemy.sql.expression.Tuple etc.

CaselIT commented 2 years ago

Hi,

I'm not sure this is a bug actually. The elements that are not re-exported are not included in the the __all__ list in the original file: https://github.com/sqlalchemy/sqlalchemy/blob/main/lib/sqlalchemy/sql/expression.py

So I think we should decide if the public interface is __all__ or whatever it's imported in the module

rectalogic commented 2 years ago

My usecase is I want to use these type definitions when typing my own code. e.g. I have some function that accepts a ColumnClause and so want to hint it:

def do_select(c: ColumnClause) -> Select:
    return select(c).select_from("user")

Unless all of these types are internal and not supposed to be used at all (in which case they should probably be renamed with a leading_ and not documented), then they should be exposed when I install the stubs.

Right now my code passes mypy unless I install the stubs, then I get dozens of failures due to all of these names in my codes hints no longer existing.

CaselIT commented 2 years ago

I probably explained myself poorly.

The type is public in the sql.elements module. What is to decide is if it's public also in sql.expression. Once it is it cannot be removed from that module without a breaking change

rectalogic commented 2 years ago

Hmm, I see. I have been using the documented type names - so sqlalchemy.sql.expression.ColumnClause instead of sqlalchemy.sql.elements.ColumnClause https://docs.sqlalchemy.org/en/14/core/sqlelement.html#sqlalchemy.sql.expression.ColumnClause

How would I even determine the correct non-reexported name to use in my own type hints other than tracing the imports thru the source code?

Anyway, changing my sample code above it now passes mypy

import sqlalchemy

CC = sqlalchemy.sql.elements.ColumnClause
CaselIT commented 2 years ago

I tend to agree that probably anything that's in expression should be exported, since the module seems mostly a public interface one. @zzzeek thoughts?

zzzeek commented 2 years ago

all of these words are supposed to be available from sqlalchemy.sql.expression, and sqlalchemy.sql.elements is an internal name. so importing from "sqlalchemy.sql.expression" should produce a name that does not produce mypy errors.

CaselIT commented 2 years ago

Should all be updated in Sqlalchemy then?

zzzeek commented 2 years ago

that was my immediate impression but with all things mypy there is some uncertainty

CaselIT commented 2 years ago

I'll update both then.

Is these some other module that may be in the same situation and should.be checked?

rectalogic commented 2 years ago

I think any module that is in the docs should export the names it documents https://docs.sqlalchemy.org/en/14/genindex.html

zzzeek commented 2 years ago

doesnt apply to the stubs here, but for SQLAlchemy itself which will be retiring the separate stubs package, there seems to be some debate over if a Python package w/ py.typed needs to do explicit re-exports: https://github.com/python/mypy/issues/8754

jshields commented 5 months ago

The documentation does give examples that yield mypy errors, such as this one with expression.FunctionElement: https://docs.sqlalchemy.org/en/20/core/compiler.html#utc-timestamp-function

CaselIT commented 5 months ago

note that the v2 documentation assumes that this package is not being used