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

Fix inaccurate docs in active_model errors [ci skip] #18099

Merged
merged 1 commit into from
Dec 22, 2014
Merged

Fix inaccurate docs in active_model errors [ci skip] #18099

merged 1 commit into from
Dec 22, 2014

Conversation

robsonmarques
Copy link
Contributor

The default value for the argument message in ActiveModel::Errors#add has a new behavior
since ca99ab2.

Before
  person.errors.add(:name, nil)
  # => ["is invalid"]
After
  person.errors.add(:name, nil)
  # => [nil]

@@ -263,7 +263,7 @@ def to_hash(full_messages = false)

# Adds +message+ to the error messages on +attribute+. More than one error
# can be added to the same +attribute+. If no +message+ is supplied,
# <tt>:invalid</tt> is assumed.
# <tt>:invalid</tt> is assumed (but not if the +message+ is nil).
Copy link
Member

Choose a reason for hiding this comment

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

I think the current documentation already cover this.

If no +message+ is supplied, <tt>:invalid</tt> is assumed.

If you pass nil you are suppling a nil message.

Copy link
Member

Choose a reason for hiding this comment

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

cc @fxn @zzak @matthewd WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

I think RF is right, saying "no +message+ is supplied" should be enough to say that passing nil is still passing something. Therefore :invalid won't be used for the message when nil is given.

Copy link
Member

Choose a reason for hiding this comment

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

A couple of minor details:

message is not a noun. You can refer "the message" with an article if "message" goes in regular font, or to message without article. For example, a user may be logged in, but in a method RDoc you'd say "user is an instance of User" (not "the user").

Also, nil should go in fixed-width font.

So, it would be

# <tt>:invalid</tt> is assumed (but not if +message+ is +nil+).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

NIce. Thanks! 😄

@rafaelfranca
Copy link
Member

Awesome! Thank you for working on this

@robsonmarques
Copy link
Contributor Author

@rafaelfranca My pleasure!

@egilburg
Copy link
Contributor

Speaking of returned value ["is invalid"] - is the exact return value significant? (as opposed of the side effect of adding the actual error to object)?

Because a structure like { name: ["is invalid"] }, or the errors object, or even the actual model instance, all seem like more sense to be returned here.

The default value for the argument `message` in
`ActiveModel::Errors#add` has a new behavior
since ca99ab2.

Before
  person.errors.add(:name, nil)
  # => ["is invalid"]

After
  person.errors.add(:name, nil)
  # => [nil]
@robsonmarques robsonmarques changed the title Fix inaccurate docs in active_model errors Fix inaccurate docs in active_model errors [ci skip] Dec 22, 2014
@zzak
Copy link
Member

zzak commented Dec 22, 2014

@egilburg I'm not sure what you mean..

@robsonmarques Thank you for the patch!

zzak pushed a commit that referenced this pull request Dec 22, 2014
…add_docs

Fix inaccurate docs in active_model errors [ci skip]
@zzak zzak merged commit d610cd0 into rails:master Dec 22, 2014
@robsonmarques robsonmarques deleted the fix_active_model_errors_add_docs branch December 22, 2014 12:04
@robsonmarques
Copy link
Contributor Author

@zzak You're welcome! 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants