Skip to content

[Polysemy] Generalise and Move the Logger Effect to polysemy-wire-zoo#2306

Merged
mdimjasevic merged 17 commits intodevelopfrom
mdimjasevic/polysemy-logger-to-lib
Apr 28, 2022
Merged

[Polysemy] Generalise and Move the Logger Effect to polysemy-wire-zoo#2306
mdimjasevic merged 17 commits intodevelopfrom
mdimjasevic/polysemy-logger-to-lib

Conversation

@mdimjasevic
Copy link
Contributor

The PR generalises and moves the Logger effect from Spar into polysemy-wire-zoo.

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.

Marko Dimjašević added 3 commits April 22, 2022 09:00
@mdimjasevic mdimjasevic temporarily deployed to cachix April 22, 2022 07:27 Inactive
@mdimjasevic mdimjasevic requested a review from isovector April 22, 2022 07:28
@pcapriotti
Copy link
Contributor

There is already a logger effect in polysemy-wire-zoo, which we use in galley and federator. Would it make sense to merge the two?

@mdimjasevic
Copy link
Contributor Author

There is already a logger effect in polysemy-wire-zoo, which we use in galley and federator. Would it make sense to merge the two?

It crossed my mind and I was lazy to look at that. But you're right, this is a good opportunity to do that so I'll do it.

@mdimjasevic mdimjasevic temporarily deployed to cachix April 22, 2022 14:03 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 22, 2022 14:03 Inactive
Copy link
Contributor

@isovector isovector 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, but I'd suggest dropping discardTinyLogs

discardLogs :: Sem (Logger msg ': r) a -> Sem r a
discardLogs = interpret $ \(Log _ _) -> pure ()

discardTinyLogs :: Sem (Logger (Log.Msg -> Log.Msg) ': r) a -> Sem r a
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 you need this function?

Copy link
Contributor Author

@mdimjasevic mdimjasevic Apr 22, 2022

Choose a reason for hiding this comment

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

This is to help GHC with type inference. If I don't use it, then GHC reports overlapping instances. An alternative is to use discardLogs @(Log.Msg -> Log.Msg) instead of discardTinyLogs.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's surprising to me. Does the polysemy plugin help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's surprising to me. Does the polysemy plugin help?

It doesn't seem to. I tried this: 1) put the plugin as a GHC option in the cabal file and make its package a dependency, 2) use discardLogs instead of discardTinyLogs where I'm hoping the plugin would help, 3) observe I get the aforementioned error on overlapping instances, e.g.:

src/Federator/MockServer.hs:93:15: error:
    * Overlapping instances for Member
                                  (Wire.Sem.Logger.Logger
                                     (System.Logger.Message.Msg -> System.Logger.Message.Msg))
                                  '[Wire.Sem.Logger.Logger msg0, Embed IO]
        arising from a use of `runWaiErrors'
      Matching instances:
        two instances involving out-of-scope types
        (use -fprint-potential-instances to see them all)
      (The choice depends on the instantiation of `msg0'
       To pick the first instance above, use IncoherentInstances
       when compiling the other instance declarations)
    * In the second argument of `(.)', namely
        `runWaiErrors @'[ValidationError, ServerError, MockException]'
      In the second argument of `(.)', namely
        `discardLogs
           . runWaiErrors @'[ValidationError, ServerError, MockException]'
      In the expression:
        runM
          . discardLogs
              . runWaiErrors @'[ValidationError, ServerError, MockException]
   |
93 |             . runWaiErrors
   |               ^^^^^^^^^^^^...
cabal: Failed to build federator-1.0.0 (which is required by brig-2.0).

@mdimjasevic mdimjasevic temporarily deployed to cachix April 25, 2022 09:15 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 25, 2022 09:27 Inactive
Marko Dimjašević added 2 commits April 25, 2022 13:41
- The interpretation is not specific to TinyLog so it has been moved to
the module where the Logger effect is defined
@mdimjasevic mdimjasevic temporarily deployed to cachix April 25, 2022 11:42 Inactive
@mdimjasevic
Copy link
Contributor Author

There is already a logger effect in polysemy-wire-zoo, which we use in galley and federator. Would it make sense to merge the two?

Done! I made the one that has been in Galley and Federator just a special case of the one I generalised from Spar.

Copy link
Contributor

@isovector isovector left a comment

Choose a reason for hiding this comment

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

lgtm

@mdimjasevic mdimjasevic temporarily deployed to cachix April 26, 2022 07:29 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 26, 2022 09:58 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 26, 2022 11:28 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 26, 2022 14:38 Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix April 28, 2022 08:57 Inactive
@mdimjasevic mdimjasevic merged commit 9b8d3ac into develop Apr 28, 2022
@mdimjasevic mdimjasevic deleted the mdimjasevic/polysemy-logger-to-lib branch April 28, 2022 18:27
@mdimjasevic mdimjasevic changed the title [Polysemy] Move the Logger Effect to polysemy-wire-zoo [Polysemy] Generalise and Move the Logger Effect to polysemy-wire-zoo Apr 28, 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