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

Split serialisation from IO #5049

Merged
merged 8 commits into from
Apr 6, 2023
Merged

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Apr 4, 2023

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

@newhoggy newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from 93bfd86 to ed76cf9 Compare April 4, 2023 08:12
@newhoggy newhoggy marked this pull request as ready for review April 4, 2023 08:24
@newhoggy newhoggy changed the title Split serialisation from io Split serialisation from IO Apr 4, 2023
writeByteStringFile fp bs = runExceptT $
handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs

writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ())
Copy link
Contributor

@carbolymer carbolymer Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Handle

where 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).

Copy link
Contributor Author

@newhoggy newhoggy Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created: #5062

@@ -244,8 +244,9 @@ runConvertToNonExtendedKey evkf (VerificationKeyFile vkf) =
writeToDisk
Copy link
Contributor

@carbolymer carbolymer Apr 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 vk

is used now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple minor changes

writeByteStringFile fp bs = runExceptT $
handleIOExceptT (FileIOError fp) $ BS.writeFile fp bs

writeByteStringOutput :: MonadIO m => Maybe FilePath -> ByteString -> m (Either (FileError ()) ())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cardano-api/src/Cardano/Api/SerialiseTextEnvelope.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/IO/Compat.hs Outdated Show resolved Hide resolved
cardano-api/src/Cardano/Api/IO/Compat.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from b54b187 to cb74808 Compare April 6, 2023 03:02
@newhoggy newhoggy requested a review from Jimbo4350 April 6, 2023 03:08
@newhoggy newhoggy dismissed Jimbo4350’s stale review April 6, 2023 03:08

Comments addressed

* writeByteStringFile
* writeByteStringOutput
* writeLazyByteStringFile
* writeLazyByteStringOutput
* writeTextFile
* writeTextOutput
…ardano.Api.SerialiseTextEnvelope has less IO code and can be freed from CPP
@newhoggy newhoggy force-pushed the newhoggy/split-serialisation-from-io branch 5 times, most recently from 46e17ec to 9caffc2 Compare April 6, 2023 05:28
* writeByteStringFileWithOwnerPermissions
* writeTextFileWithOwnerPermissions
…ringFileWithOwnerPermissions and textEnvelopeToJSON instead
@newhoggy newhoggy force-pushed the newhoggy/split-serialisation-from-io branch from 9caffc2 to 2d11855 Compare April 6, 2023 05:32
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@newhoggy newhoggy added this pull request to the merge queue Apr 6, 2023
Merged via the queue into master with commit d456afb Apr 6, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/split-serialisation-from-io branch April 6, 2023 16:15
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