Skip to content
This repository has been archived by the owner on Jun 30, 2021. It is now read-only.

add option for never deleting records from db #109

Open
wants to merge 1 commit into
base: rails3.2
Choose a base branch
from

Conversation

jefk
Copy link

@jefk jefk commented Apr 22, 2013

The default behavior of the gem will delete records when destroy is called twice. I added an option, :never_delete, so a record will never be deleted from the database by calling destroy.

I don't really like the name of the option, but I couldn't think of anything better. I'm open to other ideas.

@christopherchiu
Copy link
Collaborator

I did find it mildly confusing that calling destroy on something twice would trigger a delete. I wonder if there are people out there taking advantage of that feature. Maybe we should make this a default? I would rather not add too many options.

@donv
Copy link
Contributor

donv commented Aug 13, 2013

Calling destroy twice to force delete is bad. We have used destroy! with the exclamation mark to force deletion. Having a separate method for it is better.

@jefk
Copy link
Author

jefk commented Aug 14, 2013

It would be good to never delete by default, although I'm guessing that would cause unexpected behavior to people using the double delete feature.

@christopherchiu
Copy link
Collaborator

Yea, unfortunately this gem deals with pretty sensitive issues so switching core functionality like that can be bad. Although, the worst that could happen is things don't get deleted when they were expected to, which is much better than the opposite. Let's keep this in mind, maybe make it the default on a major version upgrade.

@zzak zzak added the Future label May 12, 2014
@zzak zzak self-assigned this May 12, 2014
@zzak
Copy link
Contributor

zzak commented May 12, 2014

Also considering this for a future version.

@zzak zzak removed their assignment May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants