Datamapper DataMapper

Comments and changes to this ticket

• You flagged this item as spam.
  

  yerejm November 29th, 2007 @ 09:07 AM

  Please disregard the first patch as junk. I have done a new patch.

  The first one is junk because it doesn't respect when nil is the value you actually want to persist and masks it with the default.

  The new patch will load up the defaults on initialize and that's that.

  One spec fails because the saving of boolean properties seems broken.

  • default_property.patch 4 KB

• You flagged this item as spam.
  

  yerejm November 29th, 2007 @ 09:12 AM

  I forgot the new model. Attached.

  • defaulter.rb 829 Bytes

• You flagged this item as spam.
  

  yerejm November 29th, 2007 @ 09:13 AM

  And here's the patch to make boolean properties save pass in case it actually is a real bug for sqlite, but this isn't under a new patch as I'm not sure if the approach is right or if it is datamapper's fault for that matter.

  • truefalse_persistence.patch 670 Bytes
• 

  Nick Plante November 30th, 2007 @ 03:01 AM

  • State changed from “new” to “open”

  Can you provide an updated summary? Sorry, a little lost :-).

• You flagged this item as spam.
  

  yerejm November 30th, 2007 @ 06:10 AM

  In summary, what's being done here is that when a property has been given a default, it could be said that when accessing that property, it should provide the default value immediately.

  class Example < DataMapper::Base

  property :blob, :string, :default => "blah"

  end

  e = Example.new

  e.blob.should == X

  In current code, X is nil. The DM object has to be saved and reloaded to see the default. This patch will make X == "blah" at first instantiation. Of course, this can be overridden with nil or whatever value the client code wishes.

  Now for the explanation of the history of this ticket.

  • property_with_default.patch is bad. Please ignore it. For some reason, I can't delete it.
  • default_property.patch is the patch I submit for consideration. Please note that I had to make an alteration to an unrelated spec (spec_schema.rb) as it tests the count of the number of tables in the schema. I changed the spec to assume the count of files in the spec/models directory is the same as the table count in the schema.
  • defaulter.rb is a new model for inclusion in spec/models/ that I used for testing
  • truefalse_persistence.patch is to fix something I found with saving boolean values in a SQLite3 database. I spec defaulting boolean properties, so those specs would fail. Looking at the code handling it in DM, I'm not sure it's DM's fault since it looks like it is using prepared statements. It seems more like the sqlite dataobject code is at fault for not handling booleans. I haven't tested this against MySQL or PostgreSQL.

  Apologies for all the noise. Lighthouse seems to be made for people who know what they're doing. :)

• You flagged this item as spam.
  

  Sam Smoot December 28th, 2007 @ 10:32 PM

  • Milestone cleared.

  yerejm, sorry it took me this long to look at this. Yes, DM had a problem with booleans at one point. I suspect that's what you were running into.

  I hate to do this, but since we're up to r651 at this point, would you be kind enough to try out your patch on the latest rev and see if it still applies/works-as-intended?

  I promise to apply right away if so. In fact, this seems important enough to include in 0.3.0...

  On a somewhat related note, at some-point we need tweak the save code to reload the attributes with database defaults. I'm not sure if we want defaults in Ruby or SQL constraints at this point, but I'm more of a "keep it in Ruby" guy myself when appropriate.

  Anyways, thanks for the contribution yerejm. Whatever you can do to help close this out (a month later) at this point would be appreciated.

• You flagged this item as spam.
  

  yerejm December 29th, 2007 @ 12:00 AM

  Things have changed enough in trunk that the patch doesn't apply properly anymore. I made some effort to massage it into r651, but the one spec fails so it seems some deeper logic has also changed in that time.

  However, in the time between submitting the patch and now, I've come to see the issue the patch addresses as a non-issue. I can't think of a realistic practical use case for it. The only time the default value matters is when a model object is a brand new record. If the record is already persisted, the default is irrelevant. When it is a new record, if you care about column value, you explicitly set it and the default irrelevant, and if you don't care, it doesn't really matter anyway. The only use case I could think up, and this was what happened when I came up with the patch, only applied in (overzealous) unit testing. Furthermore, if you're a "keep it in Ruby" guy, validation for the field that has a default will catch any flaws in the business logic anyway.

  So, I'd like to ask that this patch be killed. It's not really necessary.

• You flagged this item as spam.
  

  Sam Smoot December 29th, 2007 @ 09:49 AM

  • State changed from “open” to “hold”

  I see. Well, thanks for the effort anyways.

  We should definitely revisit this at some point, since the :default option is supported, but basically poorly specced and not even I'm sure what the result of using it should be. :-)

  If you want up-front defaults, feel free to overwrite #initialize in your models though.

  Anyways, I'll close this out for now, and maybe we'll revisit the topic in another ticket another day.

• 

  Dan Kubb (dkubb) November 29th, 2008 @ 09:12 PM

  • Tag set to “patch”
  • State changed from “hold” to “invalid”

  Marking this as invalid because this no longer applied to DM.