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

Add open method on S3Boto3StorageFile with test #1227

Closed
wants to merge 1 commit into from

Conversation

rabbit-aaron
Copy link

No description provided.

@skrat
Copy link

skrat commented Mar 28, 2023

I don't like this. open method on any kind of file handle is a non-sense. There's no (hopefuly) API out there that would do something like this. File object / value / handle, per standard practices, is a result (return value) of opening some path, URL, etc. Once it's opened (in certain mode), it can be read, written and closed. Seeking is a relic from the past, but ok, it can be useful, and it can be implemented on top of a buffer.

NOTE: your __init__ -> __initialize adds nothing, solves nothing.

@rabbit-aaron
Copy link
Author

I don't like this. open method on any kind of file handle is a non-sense. There's no (hopefuly) API out there that would do something like this. File object / value / handle, per standard practices, is a result (return value) of opening some path, URL, etc. Once it's opened (in certain mode), it can be read, written and closed. Seeking is a relic from the past, but ok, it can be useful, and it can be implemented on top of a buffer.

NOTE: your __init__ -> __initialize adds nothing, solves nothing.

File object / value / handle, per standard practices, is a result (return value) of opening some path, URL, etc

This is following its super class's design. As for what you've described, it was storage.open, which is completely different.

I believe Django do this to support instant.file_field.open

calling __init__ in open looks weird, thus the renaming.

@jschneier
Copy link
Owner

Hi and thanks for the PR. I decided to go another way (see #1321)

@jschneier jschneier closed this Oct 9, 2023
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