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

Replaced #add_indifferent_access with #stringify #142

Merged
merged 5 commits into from
Oct 28, 2014

Conversation

iainbeeston
Copy link
Contributor

This is a fix for #96

Previously, add_indifferent_access would modify the data and schema hashes in place, to make them accept strings or hashes as keys. However, there are two problems with this:

  1. It works for accessing the hash (eg. fetch or []) but it does not work if we use .has_key? or .keys
  2. It does not convert arrays of symbols into arrays of strings (which can be a problem when the symbols in the array refer to attributes, eg. required)

To resolve this, I've removed add_indifferent_access in favour of stringify, which returns a new copy of the hash, with all symbols converted to strings. This is probably a better approximation of json anyway, but it also makes accessing values from the hash easier, because we always know that we'll be dealing with strings (not symbols).

@iainbeeston
Copy link
Contributor Author

Actually, this does change the behaviour of defaults slightly. Whereas before if you insert defaults, these get added into your existing data structure, now you'll also find all symbols converted to strings in your data structure (only if you insert defaults)

@iainbeeston
Copy link
Contributor Author

This might also fix #95

@RST-J
Copy link
Contributor

RST-J commented Oct 27, 2014

I think there is a little smell when a data validation library changes the existing parts of the data it receives.
However, I guess, defaults were inserted before either as strings or symbols which did not fit to the rest of the data (unless a HashWithIndifferentAccess was passed) in one case.
So how about converting/keeping all keys to strings by default (as this is more JSONary) but provide an option to convert/keep keys to/as symbols?

@iainbeeston
Copy link
Contributor Author

Yeah, I agree - I'd rather if validation didn't change the data structure it was using.

How about we deprecate insert_defaults, but instead add a new method which doesn't validate, but just inserts the defaults and returns a new object with the defaults inserted (but nothing else)?

Or (more controversial) just remove insert_defaults. They would still be used for validation.

@RST-J
Copy link
Contributor

RST-J commented Oct 27, 2014

I would not abandon insert defaults, I think its a useful feature.
I like the idea to have an extra method which does that. But it would still be more seamless if it would happen during validation.
I also think, an important point is that insert defaults should only insert any values if validation succeeds and otherwise leave the given data unchanged (is that already the case now?).

@iainbeeston
Copy link
Contributor Author

No, I think that right now it inserts into the original as it goes through
the validations

@pd
Copy link
Contributor

pd commented Oct 27, 2014

We shouldn't break the behavior of insert_defaults without a major version tick; now that @hoxworth has added a v3 milestone, maybe this belongs there, unless there's a way to get the code simplification without breaking backwards compatibility.

👍 in general, though; if there's any discussion of a v3 roadmap, some form of "drop support for symbols" is gonna be high up on my list of suggestions. =)

@iainbeeston
Copy link
Contributor Author

I agree it's a noticeable api change.

@iainbeeston iainbeeston force-pushed the better_handling_of_symbols branch from baa4cb6 to c731fe3 Compare October 27, 2014 20:49
@iainbeeston
Copy link
Contributor Author

Right guys, I've fixed this up now so that it preserves the old behaviour. If :insert_defaults is true, then at the end of validation, any new values are copied across to the original data variable. This means that symbols stay as symbols. If anything it might be better than the original, because it doesn't change the default value proc of the data object if it is a hash.

This is a fix for voxpupuli#96

Previously, add_indifferent_access would modify the data and schema
hashes in place, to make them accept strings or hashes as keys. However,
there are two problems with this:

1. It works for accessing the hash (eg. fetch or []) but it does not
work if we use .has_key? or .keys
2. It does not convert arrays of symbols into arrays of strings (which
can be a problem when the symbols in the array refer to attributes, eg.
required)

To resolve this, I've removed add_indifferent_access in favour of
stringify, which returns a new copy of the hash, with all symbols
converted to strings. This is probably a better approximation of json
anyway, but it also makes accessing values from the hash easier, because
we always know that we'll be dealing with strings (not symbols).
No longer used, because we use stringify instead of add_indifferent_access
values added during validation, rather than replacing completely

This is a fix so that the existing behaviour of insert_default is
maintained (ie. anything that is already in the object is left as it
was), but still allowing us to use a stringified hash internally and not
modifyingt the default proc of the data object.
@iainbeeston iainbeeston force-pushed the better_handling_of_symbols branch from 35dd7f6 to e96b34f Compare October 28, 2014 14:33
@pd
Copy link
Contributor

pd commented Oct 28, 2014

👍 ✖️💯

@hoxworth
Copy link
Contributor

That's a lot of thumbs. I can't compete with that.

hoxworth added a commit that referenced this pull request Oct 28, 2014
Replaced #add_indifferent_access with #stringify
@hoxworth hoxworth merged commit 21be8b9 into voxpupuli:master Oct 28, 2014
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.

4 participants