save() behavior proposal #141

Closed
nviennot opened this Issue May 5, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@nviennot
Owner

nviennot commented May 5, 2015

I'm making this issue to plan for the removal of save(). I've been looking at open source projects using nobrainer, and it seems that people do not realize that save() does not return a boolean.

To make this non ambiguous, I think we should only have save? and save!.
However, for compatibility purposes, a configuration flag config.save_behavior would be available to the user so he can specify what nobrainer should do when receiving a save() call.
This configuration flag could be set to :raise, :save?, or :save! depending on what the user wants. The default would be :raise.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig May 5, 2015

Contributor

Would you make the same behavior for create and update, then?
On May 5, 2015 8:25 AM, "Nicolas Viennot" notifications@github.com wrote:

I'm making this issue to plan for the removal of save(). I've been
looking at open source projects using nobrainer, and it seems that people
do not realize that save() does not return a boolean.

To make this non ambiguous, I think we should only have save? and save!.
However, for compatibility purposes, a configuration flag
config.save_behavior would be available to the user so he can specify
what nobrainer should do when receiving a save() call.
This configuration flag could be set to :raise, :save?, or :save!
depending on what the user wants. The default would be :raise.


Reply to this email directly or view it on GitHub
#141.

Contributor

ajselvig commented May 5, 2015

Would you make the same behavior for create and update, then?
On May 5, 2015 8:25 AM, "Nicolas Viennot" notifications@github.com wrote:

I'm making this issue to plan for the removal of save(). I've been
looking at open source projects using nobrainer, and it seems that people
do not realize that save() does not return a boolean.

To make this non ambiguous, I think we should only have save? and save!.
However, for compatibility purposes, a configuration flag
config.save_behavior would be available to the user so he can specify
what nobrainer should do when receiving a save() call.
This configuration flag could be set to :raise, :save?, or :save!
depending on what the user wants. The default would be :raise.


Reply to this email directly or view it on GitHub
#141.

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot May 5, 2015

Owner

For update, yes.

For create, I'm not sure. Not returning the created instance is weird, so there's not going to be a create?(). Keeping create() is probably fine. renaming create() for create!() is probably not that useful.

Owner

nviennot commented May 5, 2015

For update, yes.

For create, I'm not sure. Not returning the created instance is weird, so there's not going to be a create?(). Keeping create() is probably fine. renaming create() for create!() is probably not that useful.

@ajselvig

This comment has been minimized.

Show comment
Hide comment
@ajselvig

ajselvig May 5, 2015

Contributor

renaming create() for create!() is probably not that useful.

How is that any different than renaming save() to save!() ?

Contributor

ajselvig commented May 5, 2015

renaming create() for create!() is probably not that useful.

How is that any different than renaming save() to save!() ?

@nviennot

This comment has been minimized.

Show comment
Hide comment
@nviennot

nviennot May 5, 2015

Owner

I've seen too many bugs go unnoticed because a developer used save() without checking the returned value. create() on the other hand always raises in case of an error.
At the end of the day, I just want an API that reduces the number of bugs one may have.

Owner

nviennot commented May 5, 2015

I've seen too many bugs go unnoticed because a developer used save() without checking the returned value. create() on the other hand always raises in case of an error.
At the end of the day, I just want an API that reduces the number of bugs one may have.

nviennot added a commit that referenced this issue May 19, 2015

@nviennot nviennot closed this in 1faff78 May 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment