-
-
Notifications
You must be signed in to change notification settings - Fork 413
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
Fix Optional ReqBody' (wrap value into Maybe) #1347
base: master
Are you sure you want to change the base?
Fix Optional ReqBody' (wrap value into Maybe) #1347
Conversation
I’m not certain about these two patterns: (SFalse, STrue, False) -> return . either (const Nothing) (Just . Right)
(SFalse, SFalse, False) -> return . either (const Nothing) Just But they seem to work as expected. It turned ouit that requests from the tests are having |
Hi! Is there something keeping this from going upstream? I was under the impression this was the default behavior (that one would construct a custom type with |
Yep', there is a conflict to be resolved in this PR. :) |
c766e36
to
838623a
Compare
@tchoutri I resolved the conflicts and rebased the change on top of latest master. Also I ran the tests locally with a successful result. Please have a look. |
Is there anything we can do to help maintainers move this PR forward? |
I somehow managed to miss the mention from 12 days ago. There could be a CI failure due to HsOpenSSL that I'm exploring in #1635, otherwise I don't have anything more to ask. :) |
@unclechu okay, I believe that you should bump the version of HsOpenSSL used, reading its changelog: https://flora.pm/packages/%40hackage/HsOpenSSL/0.11.7.5/changelog |
I'd like to see this PR merged - is there something I can do to help? I guess if I wanted to rebase it myself I'd have to open a separate PR? |
Closes #1346
Here is a small reproducible demonstration of the fix (requires Nix to run):
https://gist.github.com/unclechu/77ec314b8651c0c17ea4ac486fdb9804
Is resolving to
[Integer]
in server serializer. This fix makes it becomeMaybe [Integer]
instead (because it hasOptional
mode).This PR brings these changes:
Optional
ReqBody'
wrap it’s type intoMaybe
Nothing
Content-Type
but without payload resolves toNothing
Content-Type
header is provided and payload is there it resolvesJust
in case there were no parsing errorsRequired
ReqBody'
behaves as usual (not affected by this fix)