Split serialisation from IO #5049
Conversation
newhoggy
force-pushed
the
newhoggy/split-serialisation-from-io
branch
from
93bfd86
to
ed76cf9
Compare
newhoggy
requested review from
dcoutts,
Jimbo4350,
erikd,
JaredCorduan and
CarlosLopezDeLara
as code owners
| writeByteStringFile fp bs = runExceptT $ | ||
| handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs | ||
|
|
||
| writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ()) |
There was a problem hiding this comment.
The first argument is a bit unclear, it takes a moment to understand that Nothing means stdout. Maybe encoding that information in the type would be better? For example:
data OutputFileDescriptor
= OutputFile FilePath
| OutputStream Handlewhere Handle can be stdout or stderr (which doesn't really matter here).
Consequently, if you open a handle to a file path, I think the other functions, which then could operate on Handles, could be unified and simplified I think (instead of matching Maybe in every one of them).
There was a problem hiding this comment.
There have been multiple attempts to do such a thing, but they've been piecemeal.
The purpose of this PR is not to introduce such an abstraction. We already have code that uses Maybe FilePath and keeping this code to that convention minimises difference.
We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.
There was a problem hiding this comment.
We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.
@carbolymer this would be a good issue to create and tackle next.
| @@ -244,8 +244,9 @@ runConvertToNonExtendedKey evkf (VerificationKeyFile vkf) = | |||
| writeToDisk | |||
There was a problem hiding this comment.
I think this function could be expanded to accept also error type constructor and could be used everywhere when the:
firstExceptT e . newExceptT $ writeLazyByteStringFile vkf' $ textEnvelopeToJSON desc vkis used now.
There was a problem hiding this comment.
I prefer not to mingle functional code with error handling if possible.
In some of the changes I've kept the firstExceptT e . newExceptT rather than grouping them also to minimise changes.
We can group them to simplify the code.
| writeByteStringFile fp bs = runExceptT $ | ||
| handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs | ||
|
|
||
| writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ()) |
There was a problem hiding this comment.
We may introduce an appropriate abstraction in a separate PR and do it across the entire codebase to handle consistently.
@carbolymer this would be a good issue to create and tackle next.
newhoggy
force-pushed
the
newhoggy/split-serialisation-from-io
branch
from
b54b187
to
cb74808
Compare
* writeByteStringFile * writeByteStringOutput * writeLazyByteStringFile * writeLazyByteStringOutput * writeTextFile * writeTextOutput
…ardano.Api.SerialiseTextEnvelope has less IO code and can be freed from CPP
…nerPermissions for consistency
newhoggy
force-pushed
the
newhoggy/split-serialisation-from-io
branch
5 times, most recently
from
46e17ec
to
9caffc2
Compare
…orWritingWithOwnerPermission
* writeByteStringFileWithOwnerPermissions * writeTextFileWithOwnerPermissions
…ringFileWithOwnerPermissions and textEnvelopeToJSON instead
newhoggy
force-pushed
the
newhoggy/split-serialisation-from-io
branch
from
9caffc2
to
2d11855
Compare
We have functions that combined IO and serialisation. For example
writeFileTextEnvelope.This PR takes a first step in separating them so that we do IO and serialisation separately.
The separation allows the IO code and the serialisation to independently become more complicated without compounding each other.
For example on the serialisation side, we may support more serialisation formats and on the IO side we may support pipes and special
"-"files.This code simplifies #5058 and moves us towards #5016