File handling methods required for request streaming

[theseion]

See SeasideSt/Seaside#1196

[jbrichau]

Hey @theseion

Nice to see this incoming!

Regarding the addition to Grease: is there a reason you did not use the existing fileStreamOn:do:binary: method?(see https://github.com/SeasideSt/Grease/blob/master/repository/Grease-Core.package/GRPlatform.class/instance/fileStreamOn.do.binary..st)

If this is Pharo only, we might just package it in a Pharo-Seaside package instead of in Grease. If it's a Grease extension, we will want to have implementations and tests for all platforms, but it does not make sense for Squeak at least to have those and GemStone might be quite difficult as well given that the Zinc port itself is lagging behind.

[theseion]

You're right, I should have used that one. I was probably thrown off because it uses the deprecated streams and I was thinking of using FileReference. I'll change that.

Yes, this is Zinc specific, which probably means Pharo only (I saw the builds fail for GemStone already because there's a method missing on Zinc class).

Since this really is something for the Zinc-Seaside package, do you think we should call it Zinc-Seaside-Pharo?

[jbrichau]

You're right, I should have used that one. I was probably thrown off because it uses the deprecated streams and I was thinking of using FileReference. I'll change that.

Good point! We might want to fix that deprecation as well then. I just switched to Pharo8 myself so I only notice it now. I have a PR pending for Pharo9-specific Grease packages. Since Pharo7 and Pharo8 share the same Grease implementation, we might want to address it in Pharo9 only?

Yes, this is Zinc specific, which probably means Pharo only (I saw the builds fail for GemStone already because there's a method missing on Zinc class).

Since this really is something for the Zinc-Seaside package, do you think we should call it Zinc-Seaside-Pharo?

Yes, it would make sense (in the Seaside PR) to call it Zinc-Pharo-Seaside. Perhaps it is also easier to add the newTemporaryFileReference to the Zinc-Pharo-Seaside package. If we add it to Grease, we need to provide implementations for all platforms, but it is only used in the case of Zinc Seaside.

[jbrichau]

My previous comment was not accurate. You would need the write: aStringOrByteArray toFile: aFileNameString inFolder: aFolderString method (instead of the fileStreamOn:do:binary: method). The first is a write method, the second a read method. Both are using the old FileStream class, so the same comment applies anyway.

In addition, it seems more appropriate to add a decent writeStreamOn:do:binary method instead. I propose to tackle that in combination with the deprecation in #104

[theseion]

I agree with everything :)
W.r.t. #newTemporaryFileReference, I figured it would be a good addition for all platforms in any case. But I have not problem with just having it as an extension to GRPharoPlatform in the Zinc-Pharo-Seaside package.

I'll close this PR since we're handling everything in here a bit differently.

[jbrichau]

Hey @theseion
Great! Since I was looking at this anyway, I just pushed a commit 2908a8f with the filestream implementation methods. For now, only in Pharo9 but I think we can add them for Pharo7 and 8 as well. I will do that for all platforms and write some more tests to make sure and send in a PR.

[theseion]

Perfect, thanks!