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

ensure symbolic error keys remain symbols #121

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

estraph
Copy link

@estraph estraph commented Oct 22, 2017

Previously, when doing this

add_error(:foo, :bar, "baz")

one could not fetch the error by that symbol, only by string.

outcome.errors["foo"]

This PR makes a tiny change to preserve the symbol keys.

outcome.errors[:foo]

Previously, when doing this

    add_error(:foo, :bar, "baz")

one could not fetch the error by that symbol, only by string.

    outcome.errors["foo"]

This PR makes a tiny change to preserve the symbol keys.
@eugeneius
Copy link
Collaborator

Nice find, @estraph! It looks like this used to work correctly, but broke all the way back in #3 🙂

Unfortunately I don't think we can just change it back: if the only way to access errors for the last five years has been to use a string, there will be a lot of code out there that does that. If we released a new version with this change, all of that code would break unless it was also updated to use symbols.

What do you think of making ErrorHash inherit from HashWithIndifferentAccess instead? That way strings will continue to work, but symbols will work too.

@estraph
Copy link
Author

estraph commented Mar 10, 2018

Great idea, I'll give that a shot. Good point about compatibility.

@eugeneius
Copy link
Collaborator

Inheriting from HashWithIndifferentAccess was proposed in #48, and rejected with this explanation:

If you have nested hashes in your inputs, and a field in a subhash is invalid, you get nested error hashes. HashWithIndifferentAccess clobbers these subhashes and removes the ErrorsHash from them, making them normal HashWithIndifferentAccess.

That behaviour was fixed in Rails 3.2.0 by rails/rails@3d6eafe; if we want to switch to inheriting from HashWithIndifferentAccess, we would have to declare 3.2.0 to be the minimum compatible version of Active Support. That seems reasonable to me, as Rails 3.x has been unsupported for some time.

We should add a test for the behaviour of nested error hashes described in that comment too.

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.

2 participants