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

Fix #1941 - real fix for the workaround #2274

Merged
merged 2 commits into from Mar 8, 2019

Conversation

Projects
None yet
3 participants
@tchaloupka
Copy link
Contributor

commented Mar 2, 2019

This bothered me so long that I've finally looked into it and found the issue. :)

Problem was that test for isSafeSerializer is using writeValue and readValue for checking and to pass correctly it requires both to compile.
In unittests, TestSerializer is used and works both as a serializer and a deserializer, so the test passes ok.
But that is not the case for ie JsonStringSerializer which has only one of those based on the provided range type.

@tchaloupka tchaloupka changed the title Fix #1941 - reall fix for the workaround Fix #1941 - real fix for the workaround Mar 2, 2019

@tchaloupka tchaloupka force-pushed the tchaloupka:fix_1941 branch from 206a4bf to dc8fd00 Mar 2, 2019

@s-ludwig

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

Ah, good catch! I'm wondering whether the two cases should be ANDed together to not miss an unsafe deserializer part when a safe serializer part is detected. Or maybe the best approach would be to just split this up into a isSafeSerializer and isSafeDeserializer?

@tchaloupka

This comment has been minimized.

Copy link
Contributor Author

commented Mar 8, 2019

Right, splitting it up is probably the best. I've made the change.

@s-ludwig
Copy link
Member

left a comment

Thanks! Setting to auto-merge.

@s-ludwig s-ludwig added the auto-merge label Mar 8, 2019

@dlang-bot dlang-bot merged commit be3bd1d into vibe-d:master Mar 8, 2019

3 checks passed

codecov/patch Coverage not affected when comparing 419ec62...9d05440
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.