Skip to content

Comments

Wip/yehudaa/added getfo function#179

Open
yehudaan wants to merge 3 commits intojbardin:masterfrom
yehudaan:wip/yehudaa/added_getfo_function
Open

Wip/yehudaa/added getfo function#179
yehudaan wants to merge 3 commits intojbardin:masterfrom
yehudaan:wip/yehudaa/added_getfo_function

Conversation

@yehudaan
Copy link

Added getfo and get_data functions to SCPClient, added tests, documentation in README

@remram44
Copy link
Collaborator

This looks good but I had assumed from #94 that we wanted to write into existing file objects, that the caller provides, rather than in-memory ones. I am not sure about the advantage of having scp.py return a BytesIO object rather than bytes?

@yehudaan
Copy link
Author

yehudaan commented Jan 30, 2023

I just thought it'll be more simple to create the BytesIO object and return it, and let the user decide what to do with it. To me it seemed the opposite of putfo. Also this way I don't have to deal with issues like whether he gave me a BytesIO or a StreamIO...
But I can make it optional for the user to give me a BytesIO object and write to it, and if he doesn't give me one I'll create it. What do you think?

Regarding the second point, I just wanted to do the opposite of putfo, so I returned a BytesIO. But I agree that returning a bytes object is the best option - that's why I added the get_data method. Do you think I should just make getfo return the data as bytes?

@riedel
Copy link

riedel commented May 21, 2023

Why is the BytesIO object created inside the function rather then a file like object being passed into the function? I am currently using StringIO to read lines...

@yehudaan
Copy link
Author

I will add the option of supplying a BytesIO object in the function.
Regarding StringIO, this won't work in python3 because paramiko's Transport's Channel's 'recv' function returns a bytes object - so writing bytes into the StringIO will fail.

@opoplawski
Copy link

I would like to see this functionality. Any progress? Thanks.

@MarekNieslanik
Copy link

It would be great to merge these commits. Any progress? Thanks

@remram44
Copy link
Collaborator

Feel free to contribute, posting this kind of comments does not advance the patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants