Skip to content

File utilities for streaming uploads#138

Merged
jbrichau merged 7 commits into
SeasideSt:masterfrom
theseion:file-utilities-for-streaming-uploads
Jun 6, 2022
Merged

File utilities for streaming uploads#138
jbrichau merged 7 commits into
SeasideSt:masterfrom
theseion:file-utilities-for-streaming-uploads

Conversation

@theseion

@theseion theseion commented Jun 4, 2022

Copy link
Copy Markdown
Member

This PR accompanies SeasideSt/Seaside#1196.

@theseion

theseion commented Jun 4, 2022

Copy link
Copy Markdown
Member Author

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

@theseion

theseion commented Jun 4, 2022

Copy link
Copy Markdown
Member Author

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

@jbrichau

jbrichau commented Jun 5, 2022

Copy link
Copy Markdown
Member

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.

theseion added 4 commits June 5, 2022 15:56
#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

theseion commented Jun 5, 2022

Copy link
Copy Markdown
Member Author

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

@jbrichau

jbrichau commented Jun 6, 2022

Copy link
Copy Markdown
Member

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

@jbrichau

jbrichau commented Jun 6, 2022

Copy link
Copy Markdown
Member

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

theseion commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

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