On Sunday, July 18, 2010 11:59:41 am Martin Gamsjaeger wrote:
> I think the motivation behind DM not raising an exception on any save
> failure by default is a very simple and intuitive one.
>
> DM cannot assume to know exactly when a failing save can really be
> considered *exceptional* behavior.
Really?
irb(main):001:0> 1/0
ZeroDivisionError: divided by 0
from (irb):1:in `/'
from (irb):1
from /home/ruby19/bin/irb:12:in `<main>'
Who is the core library to say that this is an exception? Why not just create
an "Indefinite" constant and return that? Or take the limit and return
Infinity?
Because it IS exceptional behavior. If it's not, that's why we have catch --
and there are things we can do to avoid the exception. For example, in this
case, I could've simply checked myself if it was 0 and done something else.
> Rather than forcing the user to
> treat every single save failure as an exception (which simply isn't
> true), it decided to inform users in a friendly and more quiet way,
I'm sorry, no, it's not "more friendly" to force me to revert to C-style code,
where I have to check the return value of every other function to make sure
something didn't fail.
Things should fail early and noisily. If it truly wasn't exceptional behavior,
that's OK, I can deal with that. The minor irritation of occasionally having
to deal with an exception is well worth avoiding SILENTLY IGNORED ERRORS,
which is what DM's default behavior entails.
> that doesn't involve the overhead of raising an exception either.
This might be a valid reason. I can also see cases like make_resourceful,
where it's nice to be able to do this:
if current_object.save
...
else
...
So yes, it makes library code mildly nicer, at the cost of making application
code much more difficult to debug. I don't think that's a good trade,
especially considering that it actually makes things worse, if people were to
ever actually use raise_on_save_failure. Now, instead of my library code
looking like this:
begin
current_object.save
...
rescue whatever
...
end
It'd look like this:
old_rosf = current_object.raise_on_save_failure
if current_object.save
save_successful!
else
save_failed!
end
current_object.raise_on_save_failure = old_rosf
Wait, that's the naive, non-threadsafe way to do it. Guess I have no choice
but to do it like this:
if current_object.raise_on_save_failure
begin
current_object.save
save_successful!
rescue whatever
save_failed!
end
elsif current_object.save
save_successful!
else
save_failed!
end
This is, bar none, my least favorite thing about DataMapper. I think I might
turn raise_on_save_failure on globally and see what breaks. Short of that,
I've actually written code like this:
module Base
def self.included klass
klass.class_eval do
include DataMapper::Resource
include ClassMethods
end
end
...
# And now, how save should have worked:
def store
save || raise "Save failed!"
end
def store!
save
end
def store!!
save!
end
end
That way, I can safely use plugins which assume the default behavior, but
(hopefully) my own code will be somewhat safe.
But please, please make it the default, and provide a _separate_method_ for
people to use when they want true/false returned, preferably something that
makes it obvious how dangerous it is. Think about it -- the current behavior
is effectively setting whether saves are exceptional based on the object (or
class, or application), and not on the context -- but it's the context which
defines how errors are handled, if, indeed, they're handled at all.
Or does anyone, ever, think something like this is a good idea:
u = User.new
u.name = 'John Smith'
u.save
puts 'Saved successfully! (I hope.)'
...really? Would you ever just blindly fire off a save and not care whether it
succeeded or not?
Then why does your public API encourage this by making it so easy to do, even
by accident?
--
You received this message because you are subscribed to the Google Groups
"DataMapper" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/datamapper?hl=en.
