Rails Ruby on Rails

Comments and changes to this ticket

• 

  Eloy Duran February 3rd, 2009 @ 05:55 PM

  I would suggest to change the test test_should_remove_delete_key_from_arguments_hash_of_new_records to use '0' for the 'delete' key. Right now the record is rejected because delete has a truthy value, if you use '0' it will build and actually test what you intended to.

  Other then that I agree with the patch.

• 

  Eloy Duran February 3rd, 2009 @ 05:58 PM

  Arg, don't mind the formatting you get the point :)

• 

  DHH February 3rd, 2009 @ 06:34 PM

  • Milestone cleared.

• You flagged this item as spam.
  

  Pascal Ehlert February 3rd, 2009 @ 06:47 PM

  Haha yes, I've been pretty tired when I did the patch. Thanks for the review and please find attached the updated version.

  • new_records_with_delete_key.diff 2.4 KB
• 

  Eloy Duran February 4th, 2009 @ 08:27 AM

  I have added some documentation and fixed a few whitespace errors. I'm a whitespace fanatic :)

  +1 The patch indeed tests the expected behaviour.

  • new_records_with_delete_key.diff 3.4 KB

• You flagged this item as spam.
  

  Pascal Ehlert February 4th, 2009 @ 10:27 AM

  Excellent, thanks a lot. I thought it would be obvious that a _delete key on new records rejects them, but it's certainly good to clearly state it in the documentation.

  Where have you found a whitespace error? I'm using TextMate with two spaces indentation, so there shouldn't be any. I suppose we just have a different idea of how it should be done here, because I'm usually careful with whitespaces as well. ;-)

  Thanks.

• 

  Eloy Duran February 4th, 2009 @ 10:42 AM

  The Rails source in general uses no spaces at all for empty lines. Textmate, however, adds spaces up-to the current indentation level. I myself prefer Textmate's style, but we should follow the general Rails convention.

  The “whitespace errors” I mentioned were around your test method :-)

• You flagged this item as spam.
  

  Pascal Ehlert February 4th, 2009 @ 10:51 AM

  Totally agree and having no meaningless characters in the code does actually make more sense to me.

• 

  Repository February 6th, 2009 @ 12:48 AM

  • State changed from “new” to “committed”

  (from [455a7633dbdb295de828eb2657433d47d85eb0bc]) Nested attribute accessors should ignore new records with truthy _delete key.

  Signed-off-by: Michael Koziarski michael@koziarski.com [#1861 state:committed] http://github.com/rails/rails/co...