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

feat: support io.StringIO as fp param to disnake.File #767

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Enegg
Copy link
Contributor

@Enegg Enegg commented Sep 26, 2022

Summary

Adds support for passing io.StringIO to disnake.File
Closes #765

Checklist

  • If code changes were made, then they have been tested
    • I have updated the documentation to reflect the changes
    • I have formatted the code properly by running task lint
    • I have type-checked the code by running task pyright
  • This PR fixes an issue
  • This PR adds something new (e.g. new method or parameters)
  • This PR is a breaking change (e.g. methods or parameters removed/renamed)
  • This PR is not a code change (e.g. documentation, README, ...)

@Enegg
Copy link
Contributor Author

Enegg commented Sep 26, 2022

Might be worth considering replacing the union of Union[io.TextIOBase, io.BufferedIOBase] with io.IOBase.

@onerandomusername onerandomusername added t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes labels Sep 26, 2022
@onerandomusername onerandomusername added this to the disnake v2.7 milestone Sep 26, 2022
@shiftinv
Copy link
Member

shiftinv commented Oct 3, 2022

I don't know about this - as pyright already complains, this breaks a few places that expect bytes but now receive bytes | str. Making disnake.File generic over the self.fp type could fix typing issues but still doesn't really help enforce it at runtime.

For the io.StringIO case in particular: aiohttp effectively handles that using stream.read().encode() (code), arguably something that's easy to do in user code as well.

@Enegg
Copy link
Contributor Author

Enegg commented Oct 4, 2022

I don't know about this - as pyright already complains, this breaks a few places that expect bytes but now receive bytes | str. Making disnake.File generic over the self.fp type could fix typing issues but still doesn't really help enforce it at runtime.

For the io.StringIO case in particular: aiohttp effectively handles that using stream.read().encode() (code), arguably something that's easy to do in user code as well.

So you suggest not to change the typing, and instead have the user do something like disnake.File(BytesIO(text_file.read().encode()))?

@shiftinv
Copy link
Member

shiftinv commented Oct 5, 2022

So you suggest not to change the typing, and instead have the user do something like disnake.File(BytesIO(text_file.read().encode()))?

I can't think of many scenarios where one can get a string stream but not a bytes stream, in the simplest case it's a matter of changing open(..., "r") to open(..., "rb").
From the looks of it, this is primarily a typing issue and supporting string streams in disnake.File isn't trivial, and there's a tradeoff to be made between library complexity and usability :/
Perhaps @onerandomusername or @EQUENOS can chip in, there might be other ideas for resolving this.

@Enegg
Copy link
Contributor Author

Enegg commented Oct 8, 2022

I can't think of many scenarios where one can get a string stream but not a bytes stream

My primary use case is creating an in-memory text file from a string with io.StringIO; It's useful when message length surpasses 2000 char limit

@shiftinv shiftinv removed this from the disnake v2.8 milestone Feb 5, 2023
@Skelmis
Copy link
Contributor

Skelmis commented Feb 22, 2023

Just commenting for the record, I'd also plus 1 this feature as something I use within bots to save arbitrary writes to disk

@Enegg Enegg changed the title typing: add io.TextIOBase as supported fp for disnake.File typing: add io.StringIO as supported file-like object to disnake.File Mar 27, 2023
@Enegg
Copy link
Contributor Author

Enegg commented Mar 27, 2023

@shiftinv I forgot about this PR; how about we special-case io.StringIO as acceptable fp to File, and encode the contents to BytesIO

@shiftinv
Copy link
Member

Since this topic came up again yesterday, I looked into this more; somehow missed the last comment here, sorry.
I've partially implemented both approaches (generic file/encode bytes->str) here, and the Generic one is pretty jank, to say the least.

how about we special-case io.StringIO as acceptable fp to File, and encode the contents to BytesIO

Yeah, this seems like the better option. aiohttp ends up doing almost the same thing internally if you pass StringIO directly, so this should be fine?

@Skelmis
Copy link
Contributor

Skelmis commented May 10, 2024

Do we have anything further on this PR? Seems like a usable resolution is fairly close

@Enegg Enegg changed the title typing: add io.StringIO as supported file-like object to disnake.File feat: support io.StringIO as fp param to disnake.File Nov 14, 2024
@Enegg Enegg force-pushed the File-textIO-support branch from 317af88 to eb6551d Compare November 14, 2024 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t: enhancement New feature t: refactor/typing/lint Refactors, typing changes and/or linting changes
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

disnake.File support for io.TextIOBase files
4 participants