Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would moving it to the dev dependencies work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is needed either way. Don't fully understand why. Apparently
types_SQLAlchemy
is built from thetypeshed
repo, but only as a pip installer, and you can't have a conda installer depend on packages only available viapip
.We might be able to remove
types-pytz
as a dependency as well.What I don't understand is that we have
import sqlalchemy.engine
in the stubs, so you'd expect that if it wasn't installed, the type checkers would complain about a missing import. And I don't have it installed in my environment now, nor do we install it when we do CI, yet we get no failures.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would get failures if we enabled more strict settings. I think
ignore_missing_imports = true
would find these cases (and maybe other Any-related checks from mypy). Not sure why reportMissingImports doesn't trigger errros (maybe we need reportMissingTypeStubs).These third-party stubs are probably only important if we specifically test whether
to_sql
accepts non-sql objects as an argument (if mypy/pyright don't know what the accepted type is, they will assume it is Any/Unknown).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we'd get the failures with
ignore_missing_imports=true
as well. If I turn onreportMissingTypeStubs
, then we get complaints aboutmatplotlib
and excel related items.I think it has to do with how either (or both)
mypy
andpyright
usetypeshed
, where the types forSQLAlchemy
are defined.If you could approve/merge this PR, I will do a new release of the version I put on
pypi
yesterdayThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense