Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #9995 (new enhancement)

Opened 7 months ago

Last modified 2 months ago

[PATCH][DOCS] 'errors.add("phone_number",' - pedantic documentation enhancement

Reported by: trevor_wennblom Assigned to: core
Priority: low Milestone: 2.x
Component: ActiveRecord Version: edge
Severity: minor Keywords: docs tiny
Cc:

Description

Extremely trivial. Keep letters out of the phone number in this example. As it stands it's just validating that a single digit exists, while other crud could be in 'phone_number'. 

--- lib/active_record/validations.rb.orig       2007-10-25 23:59:32.000000000 -0500
+++ lib/active_record/validations.rb    2007-10-26 00:00:31.000000000 -0500
@@ -252,7 +252,7 @@
   #     protected
   #       def validate
   #         errors.add_on_empty %w( first_name last_name )
-  #         errors.add("phone_number", "has invalid format") unless phone_number =~ /[0-9]*/
+  #         errors.add("phone_number", "has invalid format") unless phone_number =~ /^[0-9\-]*$/
   #       end
   #
   #       def validate_on_create # is only run the first time a new object is saved

Attachments

pedantic_doc_fix.diff (0.7 kB) - added by jeremymcanally on 11/08/07 04:47:37.
Properly formed patch file

Change History

11/08/07 04:46:31 changed by jeremymcanally

While I agree it's pedantic, I don't see why this shouldn't be in there if anything to promote good practice.

11/08/07 04:47:37 changed by jeremymcanally

  • attachment pedantic_doc_fix.diff added.

Properly formed patch file

11/08/07 04:56:45 changed by developingchris

  • keywords changed from documentation patch to doc, tiny.

+1 pedantic, but still promotes a best practice

11/08/07 12:07:43 changed by manfred

-1 This is absurd. If you want to give a complete example, make it complete. What about brackets, the plus sign and the pound (for extensions)? It's better to suggest validates_format_of in this case anyway.

How about we use the following? 

def validate
  if subscribe_to_mailing_list? and email.blank?
    errors.add("email", "can't be blank when you want to subscribe to the mailing list")
  end
end

11/08/07 12:44:35 changed by norbert

  • keywords changed from doc, tiny to docs tiny.
  • summary changed from 'errors.add("phone_number",' - pedantic documentation enhancement to [PATCH][DOCS] 'errors.add("phone_number",' - pedantic documentation enhancement.

I agree with manfred, this is ridiculous. -1

The sample he posted is something I can agree with.

11/08/07 18:33:47 changed by jeremymcanally

I agree; this should be in validates_format_of. Not sure why that example is like that...

On the other hand, the suggested example doesn't provide a full context. Perhaps something like this: 

class Invoice
  belongs_to :client

  def validate
    errors.add("Not enough funds") unless client.charge(amount)
  end
end

11/09/07 13:09:17 changed by norbert

jeremymcanally, your example also uses an "out of context" method, Client#charge.

Now that I think about it, manfred's example could actually be done with 

validates_presence_of :email, :if => :subscribe_to_mailing_list?

so that's no good either.

The other validate methods for the Person class in this piece of documentation actually make sense, so I would suggest coming up with something that goes well with the current code. (No, I couldn't think of anything.)

03/27/08 16:15:51 changed by danger

+1 Since there's no consensus on a replacement for this line of code it seems best to just fix what's there.