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

stubs use implicit reexports #187

Open
rectalogic opened this issue Oct 26, 2021 · 13 comments
Open

stubs use implicit reexports #187

rectalogic opened this issue Oct 26, 2021 · 13 comments
Labels

Comments

@rectalogic
Copy link

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.

  • OS: Ubuntu 20.04
  • Python: 3.8.10
  • SQLAlchemy: 1.4.23
  • mypy: 0.910
  • SQLAlchemy2-stubs: 0.0.2a18

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.

@rectalogic rectalogic added the requires triage New issue that requires categorization label Oct 26, 2021
@zzzeek zzzeek added bug Something isn't working and removed requires triage New issue that requires categorization labels Oct 26, 2021
@CaselIT CaselIT removed the bug Something isn't working label Oct 26, 2021
@CaselIT
Copy link
Member

CaselIT commented Oct 26, 2021

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
Copy link
Author

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
Copy link
Member

CaselIT commented Oct 26, 2021

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
Copy link
Author

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
Copy link
Member

CaselIT commented Oct 26, 2021

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
Copy link
Member

zzzeek commented Oct 26, 2021

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
Copy link
Member

CaselIT commented Oct 26, 2021

Should all be updated in Sqlalchemy then?

@zzzeek
Copy link
Member

zzzeek commented Oct 26, 2021

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

@CaselIT
Copy link
Member

CaselIT commented Oct 26, 2021

I'll update both then.

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

@rectalogic
Copy link
Author

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

@zzzeek
Copy link
Member

zzzeek commented Jan 24, 2022

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: python/mypy#8754

@jshields
Copy link

jshields commented Apr 9, 2024

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
Copy link
Member

CaselIT commented Apr 11, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants