Skip to content

Fix issue with logic around type of object returned by smart_open #26

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

Merged
merged 1 commit into from
Jul 23, 2021

Conversation

lossyrob
Copy link
Member

This fixes an issue introduced in #18 with the usage of isinstance to check the return type of smart_open.open.
There was a check in place that was made under the assumption the return type would be consistent with what I was seeing in my development environment at time of coding; however in another instance I found this open returned an unexpected type for an
open file.

This change avoids having type checking interfere with runtime logic by assigning the open for file to Any, which skips further type checking.

This fixes an issue where the return type of smart_open can vary in
different environments. There was a check in place that was made under
the assumption the return type would be consistent with what I was
seeing in my development environment at time of coding; however in
another instance I found this open returned an unexpected type for an
open file.

This change avoids having type checking interefere with runtime logic
by assigning the open for file to `Any`, which skips further type checking.
@bitner
Copy link
Collaborator

bitner commented Jul 22, 2021

Hey @lossyrob is there a reason related to mypy or something that we can't just use the open within the context? ie

with open(file, "rb") as f:
    async with DB(dsn) as conn:
        await load_iterator(f, table, conn, method)

@lossyrob
Copy link
Member Author

@bitner yeah, smart_open.open describes a return type, but it's TextIOWrapper | str | None, which trips up mypy. Opening before the context expression allows type hinting against the return value, and the previous broken logic was around the return type I was seeing from smart_open.open (which was not TextIOWrapper). Seems like an instance where type hinting is used by not actually adhered to, which causes mypy to be cranky. The fix here could be

with open(file, "rb") as f:  # type:ignore
    async with DB(dsn) as conn:
        await load_iterator(f, table, conn, method)

but then the type information around f is not ignored moving forward in the code, and other "type:ignore" tags might be needed. Assigning the return value to Any making mypy basically ignore inferring information about the type.

@bitner
Copy link
Collaborator

bitner commented Jul 23, 2021

uggh. annoying, but makes sense.

@lossyrob lossyrob merged commit 360f112 into main Jul 23, 2021
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.

2 participants