Skip to content

Conversation

mdimjasevic
Copy link
Contributor

This is the first PR in a series of PRs that introduce Polysemy to Brig. It introduces the row type variable to several monads in the service.

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@@ -399,8 +399,7 @@ withOptLock u c ma = go (10 :: Int)
execDyn' e m conv cmd = recovering policy handlers (const run)
where
run = execCatch e cmd >>= either handleErr (return . conv)
handlers = httpHandlers --TODO: Figure out why it needs a constraint Typeable effs
-- handlers = httpHandlers ++ [const $ EL.handler_ AWS._ConditionalCheckFailedException (pure True)]
handlers = httpHandlers ++ [const $ Handler $ pure . const True . preview (AWS._ConditionalCheckFailedException @SomeException)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look right. The prism doesn't actually get used, so this will catch any exception, rather than the one identified by the prism.

I'm not sure why handler_ requires a typeable constraint (probably a limitation of Control.Monad.Catch), but that means this code should run in IO (or some simple monad stack over IO), and later be lifted to the AppIO/polysemy monad.

Or, for the time being, you can add a Typeable instance to AppIO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for spotting this. I'll take a look at how to fix this.

We didn't want to introduce the Typeable constraint on AppIO as that would propagate the constraint further up the call stack, and this doesn't look like the right thing to do.

Copy link
Contributor

Choose a reason for hiding this comment

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

Except for the noise, adding Typeable constraints is trivial and easy, and the reason it needs to be added explicitly is rather technical. I wouldn't spend any effort trying to avoid it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Sorry, I keep on forgetting not to rebase once a PR is under review.)

I took Paolo's suggestion and turned the helper function to run in IO instead of running in a constrained monad m. See 1abfd3c for details.

@fisx , @pcapriotti , is that OK now?

@mdimjasevic mdimjasevic force-pushed the polysemy/brig-sem-monad branch from 1ba08e7 to ddb4225 Compare February 17, 2022 12:14
- This is to work around the issue of an otherwise imposed constraint of
`Typeable effs`
@mdimjasevic mdimjasevic force-pushed the polysemy/brig-sem-monad branch from ddb4225 to 1abfd3c Compare February 17, 2022 12:16
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

Looks good!

@mdimjasevic mdimjasevic merged commit 8281461 into develop Feb 17, 2022
@mdimjasevic mdimjasevic deleted the polysemy/brig-sem-monad branch February 17, 2022 14:31
@fisx fisx mentioned this pull request Feb 18, 2022
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