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

File utilities for streaming uploads #138

Merged

Conversation

theseion
Copy link
Member

@theseion theseion commented Jun 4, 2022

This PR accompanies SeasideSt/Seaside#1196.

@theseion
Copy link
Member Author

theseion commented Jun 4, 2022

The one failure is due to an unexpected pass. This should be fixed in a separate PR.

@theseion
Copy link
Member Author

theseion commented Jun 4, 2022

This PR also deals with the issue discussed in #1159. This might not be the best solution.

@jbrichau
Copy link
Member

jbrichau commented Jun 5, 2022

Hi @theseion !
Thanks for picking this one up again.

I wonder if it would not be easier (and keep the codebase more succint) to write binaryWriteStreamOn:do: in terms of the existing writeFileStreamOn: aString do: aBlock binary: aBoolean method? Is it because passing through the filename instead of the filereference does not work for this? The advantage is also we have some tests for the existing methods.

#writeFileStreamOn:do:binary: already provides this functionality
#writeFileStreamOn:do:binary: already provides this functionality
#writeFileStreamOn:do:binary: already provides this functionality
 #writeFileStreamOn:do:binary: already provides this functionality
@theseion
Copy link
Member Author

theseion commented Jun 5, 2022

You're right, the new method is unnecessary. I've removed it.

@jbrichau
Copy link
Member

jbrichau commented Jun 6, 2022

@theseion the unexpected pass is indeed because svenvc/zinc#86 is now fixed in Pharo10. I now removed it in ce61d53

@jbrichau
Copy link
Member

jbrichau commented Jun 6, 2022

I would say we need a test for the new method.
But I will take care of it before marking a new release and merge this one in for now so we can merge the Seaside PRs

@jbrichau jbrichau merged commit 5ad8d0b into SeasideSt:master Jun 6, 2022
@theseion theseion deleted the file-utilities-for-streaming-uploads branch June 6, 2022 13:22
@theseion
Copy link
Member Author

theseion commented Jun 6, 2022

Thanks, much appreciated!

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