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

ActiveModel::Errors#generate_message without i18n_scope, and more test cases for #add #3319

Merged
merged 1 commit into from
Oct 17, 2011

Conversation

martinsvalin
Copy link
Contributor

ActiveModel::Errors#generate_message requires that the model we're adding errors to implements i18n_scope. This is not obvious from the documentation, and it is also not necessary as there are translation fallbacks that don't use i18n_scope.

The first of the three commits skips the translation defaults where i18n_scope is used if the model doesn't respond to it.

I also noticed that there were no tests on ActiveModel::Errors#add when message is a Symbol, or when message is a Proc. I wrote tests for these cases, each in it's own commit to make it easier if you'd like to pull these but not the commit dealing with i18n_scope.

Please bear in mind that, if you'd like to keep the requirement for 18n_scope on the model, the test case for when message is a Symbol won't pass. If generate_message doesn't check if the model responds, the test needs a minimal implementation of Person.i18n_scope to pass. I'll be happy to fix this.

PS. This is my first contribution to Rails. Yay! \o/
I attended an Open Source Hack Night in Stockholm on October 11. Kudos to Valtech for organizing it.

@@ -302,11 +302,11 @@ module ActiveModel

defaults = @base.class.lookup_ancestors.map do |klass|
[ :"#{@base.class.i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type}",
:"#{@base.class.i18n_scope}.errors.models.#{klass.model_name.i18n_key}.#{type}" ]
:"#{@base.class.i18n_scope}.errors.models.#{klass.model_name.i18n_key}.#{type}" ] if @base.class.respond_to?(:i18n_scope)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This if clause should actually be outside the loop. Something like:

if @base.class.respond_to?(:i18n_scope)
  defaults = @base.class.lookup_ancestors.map do |klass|
    [ :"#{@base.class.i18n_scope}.errors.models.#{klass.model_name.i18n_key}.attributes.#{attribute}.#{type}",
      :"#{@base.class.i18n_scope}.errors.models.#{klass.model_name.i18n_key}.#{type}" ]
  end
else
  defaults = []
end

@josevalim
Copy link
Contributor

This looks great. I have added one comment, could you please check it out? And congratulations on your first contribution.

@martinsvalin
Copy link
Contributor Author

Glad you liked it. You're right. Brevity is nice, but not if it means needlessly rechecking respond_to?. I've moved it outside the block.

@josevalim
Copy link
Contributor

This looks great! Could you please squash those commits into one? Thanks!

@martinsvalin
Copy link
Contributor Author

Ok, I squashed them in an interactive rebase, and then force pushed to my branch on github, but it hasn't turned up here in the pull request. I don't have a lot of experience with github pull requests, so maybe I did something I shouldn't have.

My squashed commit is available at martinsvalin@180d413

@josevalim
Copy link
Contributor

Github takes some times to update pull requests, everything looks great for me now! Thanks!

josevalim added a commit that referenced this pull request Oct 17, 2011
ActiveModel::Errors#generate_message without i18n_scope, and more test cases for #add
@josevalim josevalim merged commit 2f87b72 into rails:master Oct 17, 2011
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

2 participants