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

Refactoring-friendly FutureWork data type #1550

Merged
merged 4 commits into from
May 30, 2021
Merged

Refactoring-friendly FutureWork data type #1550

merged 4 commits into from
May 30, 2021

Conversation

fisx
Copy link
Contributor

@fisx fisx commented May 30, 2021

internal change, no need to show in the release notes. it's pretty small and self-explanatory.

@fisx fisx requested a review from smatting May 30, 2021 14:38
-- Example:
-- >>> let (FutureWork @'LegalholdPlusFederationNotImplemented -> _remoteUsers, localUsers)
-- >>> = partitionRemoteOrLocalIds domain qualifiedUids
newtype FutureWork label payload = FutureWork payload
Copy link
Contributor

@smatting smatting May 30, 2021

Choose a reason for hiding this comment

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

If you plan to use the payload then uwrapping the newtype is a bit noisy

let FutureWork x' = FutureWork @'FeatNotImplemented x

If you also provide an unwrapper FutureWork -> payload and combine wrapper and unwrapper for convenience, this degenerates to

  let  futurework :: forall a b. b -> b
       futurework = id

  let x = futurework @'FeatNotImplemented (42 :: Int)

Having FutureWork type however has the advantage that you can pass futurework-values to functions in a typesafe way, so I favor the version with the type.

@fisx fisx merged commit d3caf83 into develop May 30, 2021
@fisx fisx deleted the data-futurework branch May 30, 2021 17:27
@@ -1000,7 +1001,7 @@ withValidOtrRecipients protectee clt cnv rcps val now go = do
Data.deleteConversation cnv
throwM convNotFound
-- FUTUREWORK(federation): also handle remote members
localMembers <- Data.members cnv
(FutureWork @'LegalholdPlusFederationNotImplemented -> _remoteMembers, localMembers) <- (undefined,) <$> Data.members cnv
Copy link
Member

Choose a reason for hiding this comment

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

Does this have to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you prefer ()? I think I prefer undefined because it makes sure this makes it more clear it shouldn't be used anywhere. Perhaps there is a better idiom yet to be found though; I tried something diferent here

@fisx fisx mentioned this pull request May 31, 2021
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