Skip to content

Brig Polysemy: Port UserPendingActivationStore to polysemy#2636

Merged
fisx merged 15 commits intowireapp:developfrom
isovector:brig-upa-store-effect
Aug 26, 2022
Merged

Brig Polysemy: Port UserPendingActivationStore to polysemy#2636
fisx merged 15 commits intowireapp:developfrom
isovector:brig-upa-store-effect

Conversation

@isovector
Copy link
Contributor

@isovector isovector commented Aug 21, 2022

This PR moves Brig.Data.UserPendingActivation into a polysemy effect Brig.Sem.UserPendingActivationStore.

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.

@isovector
Copy link
Contributor Author

@mdimjasevic @pcapriotti please take a look!

@pcapriotti
Copy link
Contributor

There is a similar abstraction for paging in galley. See Galley.Effects.Paging, and Galley.Cassandra.Paging. These are not using dynamic typing, but instead require a type variable for the paging implementation. I'm not sure I like the dynamic typing trick with unsafeCoerce. Honestly, keeping the type argument around is not that much trouble. I think we should choose one of the two approaches and use it in all cases.

@isovector
Copy link
Contributor Author

Thanks for the pointers to Galley.Effects.Paging --- I like that approach more too. I'll change this one.

@isovector isovector marked this pull request as draft August 22, 2022 07:46
@isovector
Copy link
Contributor Author

Depends on #2648.

@isovector isovector marked this pull request as ready for review August 24, 2022 23:49
@mdimjasevic mdimjasevic added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Aug 25, 2022
Copy link
Contributor

@mdimjasevic mdimjasevic 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
Copy link
Contributor

@isovector , the branch fails to build Galley integration tests:

#10 299.1 [33 of 33] Compiling Main             ( test/integration/Main.hs, /wire-server/dist-newstyle/build/x86_64-linux/ghc-8.10.7/brig-2.0/x/brig-integration/noopt/build/brig-integration/brig-integration-tmp/Main.o, /wire-server/dist-newstyle/build/x86_64-linux/ghc-8.10.7/brig-2.0/x/brig-integration/noopt/build/brig-integration/brig-integration-tmp/Main.dyn_o )

#10 299.2 

#10 299.2 test/integration/Main.hs:160:64: error:

#10 299.2     • Overlapping instances for Polysemy.Internal.Union.Member

#10 299.2                                   (Brig.Sem.UserPendingActivationStore.UserPendingActivationStore

#10 299.2                                      p0)

#10 299.2                                   '[Brig.Sem.UserPendingActivationStore.UserPendingActivationStore

#10 299.2                                       polysemy-wire-zoo-0.1.0:Wire.Sem.Paging.Cassandra.InternalPaging,

#10 299.2                                     polysemy-wire-zoo-0.1.0:Wire.Sem.Now.Now,

#10 299.2                                     Brig.Sem.CodeStore.CodeStore,

#10 299.2                                     Polysemy.Embed.Type.Embed

#10 299.2                                       cql-io-1.1.1:Database.CQL.IO.Client.Client,

#10 299.2                                     Polysemy.Embed.Type.Embed IO, Polysemy.Final.Final IO]

#10 299.2         arising from a use of ‘sitemap’

#10 299.2       Matching instances:

#10 299.2         two instances involving out-of-scope types

#10 299.2         (use -fprint-potential-instances to see them all)

#10 299.2       (The choice depends on the instantiation of ‘p0’

#10 299.2        To pick the first instance above, use IncoherentInstances

#10 299.2        when compiling the other instance declarations)

#10 299.2     • In the second argument of ‘($)’, namely

#10 299.2         ‘sitemap @BrigCanonicalEffects’

#10 299.2       In the third argument of ‘assertEqual’, namely

#10 299.2         ‘(pathsConsistencyCheck . treeToPaths . compile

#10 299.2             $ sitemap @BrigCanonicalEffects)’

#10 299.2       In the second argument of ‘($)’, namely

#10 299.2         ‘assertEqual

#10 299.2            "inconcistent sitemap" mempty

#10 299.2            (pathsConsistencyCheck . treeToPaths . compile

#10 299.2               $ sitemap @BrigCanonicalEffects)’

#10 299.2     |

#10 299.2 160 |               (pathsConsistencyCheck . treeToPaths . compile $ Brig.API.sitemap @BrigCanonicalEffects),

#10 299.2     |                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@isovector
Copy link
Contributor Author

@mdimjasevic should be fixed now!

@fisx fisx merged commit d2d75ba into wireapp:develop Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants