-
Notifications
You must be signed in to change notification settings - Fork 200
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
Allow pathTo
and parse
to work with Maybe (Id record)
#1691
base: master
Are you sure you want to change the base?
Conversation
@mpscholten I've got the tests red, which is good - as I haven't fixed the route parsing yet 😅 . Can you please point me in the right direction for the parsing part |
I suspect it's here. Looks scary! 😨 |
So if I understand the code correctly, if we wanted to return a Just [here] we'd have to do ( Line 288 in 1c8756d
- Just uuid -> uuid |> unsafeCoerce |> Right
+ Just uuid -> Just uuid |> unsafeCoerce |> Right But we can't know when it's a Maybe and when a regular. So one option would be keeping the |
IHP/RouterSupport.hs
Outdated
Nothing -> | ||
-- We couldn't parse the UUID, so try Maybe (Id record), | ||
-- where we have a @Just@ prefix before the UUID. | ||
queryValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpscholten Is there a way to debug such a function and have it print the value of queryValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I've just fixed it "Just"
not "Just "
. But still would be nice to know if there's a way to debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mpscholten It's indeed a tricky problem. Is there a way to debug/print the values in those functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After fiddling with the parser, I think it would be easier to keep the
Tests are passing now 😄 |
pathTo
and parse
to work with Maybe (Id record)
This implementation is inconsistent with how other This part of IHP is a bit tricky as you've seen already :D But there's likely a way we get this consistent with the other Maybe implementations |
I was looking at the code some more, but I don't know how to proceed. Maybe I'm completely off, but I remember reading somewhere that in the past there was In short, I'm happy I was able to tackle it up to this point, but will need help/ someone to take it from here ✌️ |
Yeah, this is a tricky problem. I've already tried it myself yesterday and couldn't find a quick solution so far. Will take a deeper look soon |
I just hit this one when implementing a Chat (with ihp openAI) on my app. Tried:
Since it's not currently working, as a workaround I can change this to :
|
Actually, I don't need a workaround (or this feature). I can simply go with
So I guess this feature is still needed from time to time, but likely there are workarounds. |
I run into this problem as well every once in a while. Hope we can fix this at some point 👍 I usually follow the same workaround |
Just ran into this - I ended up converting everything in actions to be |
Yes. The current implementation of AutoRoute doesn't allow for it. It likely needs an internal refactoring to fix this issue |
fixes #1671