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

Add spec reproducing issue with custom type with and nested hashes validation #395

Closed
wants to merge 6 commits into from

Conversation

edgenard
Copy link

@edgenard edgenard commented Jan 28, 2022

This is meant to reproduce an issue where custom types in nested hashes do not take effect.
This is my first time with this project, I hope the spec is in the right place. If not I'd be happy to move it to the appropriate place.

This PR is related to this Issue: #396

I tried but could not figure out how to get it to pass.

@edgenard edgenard requested a review from solnic as a code owner January 28, 2022 01:58
@edgenard edgenard changed the title Add spec reproducing issue with custom type and array validation Add spec reproducing issue with custom type with hash validation Jan 28, 2022
Copy link
Member

@flash-gordon flash-gordon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I left a comment, can you also mark the spec as pending so that we can merge it in?

spec/integration/schema/macros/hash_spec.rb Outdated Show resolved Hide resolved
@edgenard edgenard changed the title Add spec reproducing issue with custom type with hash validation Add spec reproducing issue with custom type with and nested hashes validation Jan 28, 2022
@edgenard
Copy link
Author

@flash-gordon I tried to get the spec to pass but had a very hard time following the code, if you could point me in the right direction I'd be happy to try again.

For example, what file or files would need to change to make it pass.

@flash-gordon
Copy link
Member

I didn't mean making it pass :) Just make it pending by changing specify to xspecify, it'll stop failing the CI.

@edgenard
Copy link
Author

I didn't mean making it pass :) Just make it pending by changing specify to xspecify, it'll stop failing the CI.

I added the pending to the spec.

But I'd be happy to work on it if someone could point me in the right direction.

@solnic
Copy link
Member

solnic commented Jan 30, 2022

Your type crashes, that's why it doesn't work:

(byebug) Test::ExpirationDate["2021-11-11T00:00:00+00:00"]
*** Dry::Types::CoercionError Exception: undefined method `to_time' for "2021-11-11T00:00:00+00:00":String
Did you mean?  to_i

If you change it so that it does DateTime.parse(value), the spec will pass.

@flash-gordon it seems like Safe constructor rescues silently this crash though - THAT is probably a bug 🙂

@edgenard
Copy link
Author

Your type crashes, that's why it doesn't work:

(byebug) Test::ExpirationDate["2021-11-11T00:00:00+00:00"]
*** Dry::Types::CoercionError Exception: undefined method `to_time' for "2021-11-11T00:00:00+00:00":String
Did you mean?  to_i

If you change it so that it does DateTime.parse(value), the spec will pass.

@flash-gordon it seems like Safe constructor rescues silently this crash though - THAT is probably a bug 🙂

Thank you! I think that definitely cleared it up. And yeah the silent failure is certainly confusing to users of the library.

I'll close this PR.

@edgenard edgenard closed this Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants