Conversation
akshaymankar
force-pushed
the
akshaymankar/servantify-self
branch
4 times, most recently
from
4870d1c to
9c3f0cf
Compare
akshaymankar
force-pushed
the
akshaymankar/servantify-self
branch
from
9c3f0cf to
73df876
Compare
Also add instnce for AsUnion '[..] Bool back.
Required by `AsUnion`
Only `FromJSON` needs to be backwards compatible as this object is consumed as request body by brig. So, breaking `ToJSON` like this shouldn't cause any issues.
akshaymankar
force-pushed
the
akshaymankar/servantify-self
branch
from
73df876 to
9b5aabd
Compare
|
The |
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
Looks good!
As commented below, I think we could get rid of the boilerplate (which is quite a lot!) by changing the result types to include all responses (including the successful ones) and deriving AsUnion via GenericAsUnion.
If you prefer to keep the Maybe ErrorType pattern, you can also use GenericAsUnion, by deriving an AsUnion instance for the error type, and then having a couple lines of boilerplate to combine it into an instance for Maybe (using eitherToUnion and eitherFromUnion).
This is all a bit unfortunate, but managing error types is always a pain. Once we switch to using polysemy in brig, we might be able to improve the situation.
libs/wire-api/src/Wire/API/User.hs
Outdated
| instance AsUnion PutSelfResponses (Maybe UpdateProfileError) where | ||
| toUnion (Just ProfileNotFound) = Z (I mkErrorDescription) | ||
| toUnion (Just DisplayNameManagedByScim) = S (Z (I mkErrorDescription)) | ||
| toUnion Nothing = S (S (Z (I ()))) | ||
| fromUnion (Z (I _)) = Just ProfileNotFound | ||
| fromUnion (S (Z (I _))) = Just DisplayNameManagedByScim | ||
| fromUnion (S (S (Z (I _)))) = Nothing | ||
| fromUnion (S (S (S x))) = case x of |
There was a problem hiding this comment.
This should be probably easy to automate using GenericAsUnion.
You could also consider creating a type like UpdateProfileResult with 3 constructors instead, and then you could directly use deriving via to get the AsUnion instance.
There was a problem hiding this comment.
Thanks for the tip. I created maybeToUnion and maybeFromUnion (this one uses eitherFromUnion) to define these instances. Let me know if they look ok.
The test was using a hardcoded name for a user and searching for the user. Since the DB is not cleaned up after/before every test run, this causes the new users to not show up in the search results.
akshaymankar
force-pushed
the
akshaymankar/servantify-self
branch
from
f26fab8 to
244cdb4
Compare
akshaymankar
force-pushed
the
akshaymankar/servantify-self
branch
from
194c184 to
eaf73a4
Compare
pcapriotti
left a comment
pcapriotti
left a comment
There was a problem hiding this comment.
Looks good. It's a bit unfortunate that GHC doesn't let you define an instance for a type family application, even when it fully reduces. I'm not sure why this restriction exists.
I found this when I was confused about the error: https://gitlab.haskell.org/ghc/ghc/-/issues/3485 |
In that example the type family is stuck, and I can understand that something like that would not be allowed. However, if the type family application computes to something that is accepted as as the type of an instance, why not accept the application itself? |
2 participants
https://wearezeta.atlassian.net/browse/SQCORE-1167
Checklist
changelog.d.