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

Write failing test for coercion on initialization #472

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bobbytables
Copy link

@bobbytables bobbytables commented Jan 2, 2019

I described part of the piece I was trying to do in #471 but I didn't explain well enough. This is really for coercion on initialization of Dash with coercion included, these cases show what I mean.

@bobbytables
Copy link
Author

I'm seeing a test fail when I update the code to do what I was desiring, which tells me this is intended. So now I ask: Why?

Test that fails: https://github.com/intridea/hashie/blob/master/spec/hashie/extensions/coercion_spec.rb#L298

3) Hashie::Extensions::Coercion#coerce_key coercing core types does not coerce nil
     Failure/Error: expect(instance[:foo]).to_not eq('')

       expected: value != ""
            got: ""

       (compared using ==)
     # ./spec/hashie/extensions/coercion_spec.rb:323:in `block (4 levels) in <top (required)>'

This is weird to me, why not convert nil on core types when its explicit?

@dangerpr-bot
Copy link

dangerpr-bot commented Jan 2, 2019

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#472](https://github.com/intridea/hashie/pull/472): Write failing test for coercion on initialization - [@bobbytables](https://github.com/bobbytables).

Generated by 🚫 Danger

@bobbytables
Copy link
Author

I made my tests pass, but like I said, it breaks another so this was intended in some way. So I'll leave this open to discuss why we're not converting core objects when they're nil. This was a pretty big curveball for me so I'd love to know why and maybe we can sand this part down and make it at least more clear in the README.

@dblock
Copy link
Member

dblock commented Jan 3, 2019

Good work. Lets see what @michaelherold has to say!

@bobbytables
Copy link
Author

Ping, @michaelherold 😄

@michaelherold
Copy link
Member

I'm just taking a look at this; sorry for the delay! In your original issue you're talking about a Trash but what you're dealing with here is a Dash with the Coercion mix-in added. Can you explain the mismatch there?

I ask because there's a difference between coercing a value and setting a default when missing. Your PR here is talking about coercing a value, which requires a value to coerce. Setting a default should happen when the value is missing.

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.

None yet

4 participants