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

Don't change default value for Sequel::Model.raise_on_save_failure #11

Closed
wants to merge 1 commit into from

Conversation

dlee
Copy link

@dlee dlee commented Jan 30, 2013

Sequel documentation explicitly states that the default for Sequel::Model.raise_on_save_failure is true. We should respect that, unless there's a very compelling reason to override the default at the cost of confusing users.

Sequel documentation explicitly states that the default for
Sequel::Model.raise_on_save_failure is true. We should respect that,
unless there's a very compelling reason to override the default at the
cost of confusing users.
@JonathanTron
Copy link
Member

Hi @dlee, the initial reason was to follow the behavior of ActiveRecord, this was added to not confused the users switching to Sequel. Moreover, raising on save failure goes against a lot of tutorial, books, recommendation available about developping with Rails.

I see your point and had the same reaction initially, but after a handful of emails about guys surprised by the raise behavior, I think this is the way to go. Granted this gem is to help with integration of Sequel in Rails I think we should try to respect the default expected behavior of Rails.

Do you think a more clear info about this change of default Sequel behavior is acceptable?

@dlee
Copy link
Author

dlee commented Jan 30, 2013

I would actually disagree that the point of sequel-rails is to respect the default behavior of Rails... the whole point of using this gem is because people want to use Sequel instead of ActiveRecord.

Those who are surprised by Sequel's behavior should not be using this gem. If they're surprised about the raise-on-save-failure behavior, they have a lot more surprises waiting for them.

@dlee
Copy link
Author

dlee commented Jan 30, 2013

Also, if you disagree with Sequel's default behavior, I think you should either:

  1. Try to convince Jeremy to change Sequel's default behavior
  2. Put something about the flag in this gem's docs

In both cases, I don't think we should change the default behavior of Sequel; it will totally catch Sequel users off-guard.

@JonathanTron
Copy link
Member

I see your point here, being a long time Sequel user myself. But I don't think this is so confusing for Sequel user as it is for Rails user using the gem.

The mere fact that Sequel has the option to disable it means at some point the need for this behavior was here.

I need more time to think about it and when to do such a change, because of the impact on actual users.

Could be great to hear more opinions on this.

Anyway, thanks for reporting your concerns about this.

@dlee
Copy link
Author

dlee commented Jan 30, 2013

But I don't think this is so confusing for Sequel user as it is for Rails user using the gem.

Both Sequel users entering the Rails world, and Rails users wanting to use Sequel would expect Sequel to behave like Sequel.

The mere fact that Sequel has the option to disable it means at some point the need for this behavior was here.

I completely agree, but let the user decide. Leaving it alone is an implicit choice to use Sequel's default.

Also, if you want to be consistent with your desire to make Sequel behave like Rails, there are other things in Sequel that should be configured to behave more like AR besides this one flag.

Anyway, thanks for reporting your concerns about this.

Thanks for maintaining this gem and providing fast and thoughtful responses!

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

Successfully merging this pull request may close these issues.

2 participants