Fix sample app: nested paths during decoding, supporting optional values#28
Conversation
|
Thanks for catching and fixing this! One of the PRs that got merged since yesterday must've broken the demo app. And I suspect is my PR here #22 but it's strange that none of the tests broke. Give me a moment to double-check which code path that the previous PR removed was handling this use case just to make sure we're fixing this for good. |
|
Feel free to merge if this works for you 👍 |
icanzilb
left a comment
icanzilb
left a comment
There was a problem hiding this comment.
It looks good and it fixes the bug, the only ask I have is to use the unit test to do few more checks
DivineDominion
changed the title
|
@icanzilb PTAL |
icanzilb
left a comment
icanzilb
left a comment
There was a problem hiding this comment.
This looks good — it fixes a few things off the issues list and that's great!
I'll have to sit down and get a good overview of where we come short while encoding/decoding — e.g. I had no idea the *IfPresent methods were needed at all...
|
@icanzilb Yeah I just noticed via caveman debugging that none of the others were ever called. I do wonder what the default protocol implementations do -- they don't use the generic encoder/decoder, that's for sure 🤔 I believe the next best thing is to duplicate the single optional string test case for all primitive values, and group that in a separate test case file for overview. Or maybe generate the code via Sourcery, if that's you thing :) But to be 100% certain what happens when, we need to cover more permutations, which is quite annoying. I envy Apple and the fact that they don't test :-----) |
|
@DivineDominion that's not entirely true, there's plenty of tests and I do intend to "snatch" them #20 |
3 participants
Fixes #27
To be honest, I don't quite understand what's going on to make this happen, and what the implications are.
I'm fiddling around with tests to reproduce the problem and catch it there, too.
In the meantime, pointers and feedback would be welcome :)