Skip to content

typing: improve type coverage in sqlalchemy.sql.base for issue #6810#12707

Closed
kapildagur wants to merge 3 commits intosqlalchemy:mainfrom
kapildagur:typing/sqlalchmey-sql-base
Closed

typing: improve type coverage in sqlalchemy.sql.base for issue #6810#12707
kapildagur wants to merge 3 commits intosqlalchemy:mainfrom
kapildagur:typing/sqlalchmey-sql-base

Conversation

@kapildagur
Copy link
Copy Markdown
Contributor


✅ typing: improve type coverage in sqlalchemy.sql.base for issue #6810

Overview

This pull request enhances the type coverage of the sqlalchemy.sql.base module as part of the gradual migration toward complete static typing under --strict mode (#6810).

The typing improvements were made with careful consideration of:

  • Mypy --strict compatibility

  • Runtime behavior preservation

  • Avoidance of external API typing regressions

  • Maintaining test and slot safety

  • Isolating downstream typing impact


✅ Validation Checklist

Checkpoint Status Notes
mypy --strict ✅ Passed No type errors in sql/base.py
flake8 ✅ Passed Style conforms to SQLAlchemy guidelines
black ✅ Passed File formatted
zimports ✅ Passed Imports sorted and grouped properly
slotscheck ✅ Passed slots declared where expected
Unit tests ✅ Passed No behavior changed; type-only changes
Type coverage increased ✅ Yes sql/base.py is now strictly typed

Copy link
Copy Markdown
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

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

great work. I've left some comments but the general code is great, thanks

Comment thread lib/sqlalchemy/sql/base.py Outdated
Comment thread lib/sqlalchemy/sql/base.py
Comment thread lib/sqlalchemy/sql/base.py Outdated
Comment thread lib/sqlalchemy/sql/base.py
Comment thread lib/sqlalchemy/sql/base.py
Comment thread lib/sqlalchemy/sql/base.py Outdated
Comment thread lib/sqlalchemy/sql/base.py
Comment thread lib/sqlalchemy/sql/base.py Outdated
Comment thread lib/sqlalchemy/sql/base.py Outdated
@kapildagur
Copy link
Copy Markdown
Contributor Author

Hii @CaselIT ,

While testing the latest changes, I noticed that an unused # type: ignore in selectable.py (line 1349) is causing a mypy --strict error.

This is currently breaking the GitHub workflow for the raised PR. Would it be okay to remove this in the same PR to keep the CI passing cleanly?

Just wanted to confirm before making that change. Thanks!

@CaselIT
Copy link
Copy Markdown
Member

CaselIT commented Jul 14, 2025

Sure thanks

@kapildagur
Copy link
Copy Markdown
Contributor Author

Hi @CaselIT,

I've made all the suggested changes and also fixed the GitHub workflow issue by removing the unused # type: ignore comment in selectable.py.

When you have a chance, please feel free to run the workflow and review the PR — let me know if there's anything else that needs adjustment or improvement.

Thanks again for your guidance!

@CaselIT CaselIT requested a review from sqla-tester July 14, 2025 19:36
Copy link
Copy Markdown
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

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

OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision 7374212 of this pull request into gerrit so we can run tests and reviews and stuff

@sqla-tester
Copy link
Copy Markdown
Collaborator

New Gerrit review created for change 7374212: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6028

@kapildagur
Copy link
Copy Markdown
Contributor Author

Good Morning @CaselIT,

I checked the CI and noticed the Python 3.7 job is failing due to the use of Final, which was introduced in Python 3.8(docs).

To keep compatibility, I’ll remove the Final annotations from this PR. Just wanted to confirm that approach before pushing the update.

@CaselIT
Copy link
Copy Markdown
Member

CaselIT commented Jul 15, 2025

It's fine to keep, I 'll adjust as needed in gerrit next. Thanks for your contribution!

@sqla-tester
Copy link
Copy Markdown
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6028 has been merged. Congratulations! :)

@sqla-tester
Copy link
Copy Markdown
Collaborator

Michael Bayer (zzzeek) wrote:

this needs to pass pep484 in order to merge

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6029

@kapildagur kapildagur deleted the typing/sqlalchmey-sql-base branch July 20, 2025 04:15
@kapildagur kapildagur restored the typing/sqlalchmey-sql-base branch July 21, 2025 05:37
@kapildagur
Copy link
Copy Markdown
Contributor Author

Hii @CaselIT,

I saw your recent commit on sql.base — thanks for pointing it out.

I was busy with exams for the last three weekends, so I couldn’t propose the PR as planned. I’m free now and will try to propose it this weekend.

@kapildagur
Copy link
Copy Markdown
Contributor Author

This weekend I'm planning for sql.compiler, Waiting for your approval...
Thanks

@kapildagur kapildagur deleted the typing/sqlalchmey-sql-base branch July 21, 2025 06:00
@CaselIT
Copy link
Copy Markdown
Member

CaselIT commented Jul 21, 2025

sure

@sqla-tester
Copy link
Copy Markdown
Collaborator

Federico Caselli (CaselIT) wrote:

recheck pep484

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6029

@sqla-tester
Copy link
Copy Markdown
Collaborator

Federico Caselli (CaselIT) wrote:

pep484 recheck

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6029

@sqla-tester
Copy link
Copy Markdown
Collaborator

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6029 has been merged. Congratulations! :)

sqlalchemy-bot pushed a commit that referenced this pull request Jul 22, 2025
improve type coverage in `sqlalchemy.sql.base`

References: #6810
Closes: #12707
Pull-request: #12707
Pull-request-sha: 7374212

Change-Id: Ied0676f420bc27ae033f0a5e6e22d806d20f4404
(cherry picked from commit ab3de7b0800a17927e09de6d570b906303897c58)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants