Skip to content

Conversation

@Jimbo4350
Copy link
Contributor

Changelog

- description: |
   `RIO` propagation 2025-05-09
  type:
  - breaking    
  - refactoring   

Context

Additional context for the PR goes here. If the PR fixes a particular issue please provide a link to the issue.

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated. See Running tests for more details
  • Self-reviewed the diff

@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch 2 times, most recently from 88fea17 to 5fb32e1 Compare May 16, 2025 19:52
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch from 5fb32e1 to 38ef900 Compare May 19, 2025 18:13
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch from 38ef900 to 0230e6b Compare May 19, 2025 19:16
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch from 0230e6b to f477119 Compare May 19, 2025 19:38
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch 7 times, most recently from d2956b7 to 08f0d8c Compare May 20, 2025 13:11
@Jimbo4350 Jimbo4350 marked this pull request as ready for review May 20, 2025 13:11
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch 2 times, most recently from 1d026b2 to a904df9 Compare May 20, 2025 19:10
fromEitherCli
:: forall e m a
. (HasCallStack, MonadIO m, Show e, Typeable e, Error e)
:: (HasCallStack, MonadIO m, Show e, Typeable e, Error e)
Copy link
Contributor

@carbolymer carbolymer May 20, 2025

Choose a reason for hiding this comment

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

Those foralls were here on purpose. Now the order of type variables is sub-optimal. The problem here is that the previous version was allowing to use TypeApplications to enforce the error type i.e.

fromEitherCli @(FileError ()) foo

Now with your version you need to do:

fromEitherCli @_ @(FileError ()) foo

which is a tad inconvenient.

Suggested change
:: (HasCallStack, MonadIO m, Show e, Typeable e, Error e)
:: (HasCallStack, Show e, Typeable e, Error e, MonadIO m)

This applies to all the functions with removed foralls.

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 actually didn't notice this. I will revert the changes.

-> m a
fromExceptTCli = withFrozenCallStack $ fromEitherIOCli . runExceptT

handleFileErrorCli
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this function? You can use fromEitherIOCli @(FileError ()) instead. This seems to be a needless specialisation.


let regCert = makeStakeAddressRegistrationCertificate req

fromEitherIOCli @(FileError ()) $
Copy link
Contributor

@carbolymer carbolymer May 20, 2025

Choose a reason for hiding this comment

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

What's wrong with type applications?

@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch 2 times, most recently from 05d80e0 to 25bce52 Compare May 21, 2025 19:59
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch from 25bce52 to a166aeb Compare May 22, 2025 13:01
@Jimbo4350 Jimbo4350 enabled auto-merge May 22, 2025 13:01
@Jimbo4350 Jimbo4350 force-pushed the jordan/rio-propagation-20250509 branch from a166aeb to 96c3c08 Compare May 22, 2025 14:56
@Jimbo4350 Jimbo4350 added this pull request to the merge queue May 22, 2025
Merged via the queue into master with commit d03b5d0 May 22, 2025
25 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/rio-propagation-20250509 branch May 22, 2025 16:11
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