Skip to content

Raise if save fails, even when there are no errors#1

Open
FeeJai wants to merge 4 commits intodtao:masterfrom
Politicon:master
Open

Raise if save fails, even when there are no errors#1
FeeJai wants to merge 4 commits intodtao:masterfrom
Politicon:master

Conversation

@FeeJai
Copy link

@FeeJai FeeJai commented Jan 22, 2015

Throw :halt in data mapper hooks causes saves to fail, but no errors in the list. Same does cascaded saving. At the moment, dm noisy failures makes this look like a success. Please fix and release a new version, your tool is awesome

@dtao
Copy link
Owner

dtao commented Feb 11, 2015

I think I need to investigate this. It's been a while since I've touched this project, so I don't really remember much about the implementation unfortunately. However, I do vaguely recall putting that logic there for some reason. Let me look into it and revisit this PR.

@dtao
Copy link
Owner

dtao commented Feb 11, 2015

Could you do me a favor? Please add a test case to this PR that demonstrates where DM fails to save a record and the current version erroneously returns true. In other words, a case where the save fails but the errors array is still empty. That will help me to understand the bug that's being fixed here.

@dtao
Copy link
Owner

dtao commented Feb 11, 2015

Also, as a totally unrelated note: I think (though I won't claim to be an expert) that it's customary to leave version bumps to library authors/maintainers. That is to say, I don't think a version change belongs in a PR. Even if I were to merge this change and then immediately push a version bump, I think that the decision to bump the version ought to be separate from the decision to merge the PR.

Don't want you to think I'm offended or anything—I really don't care—just an FYI.

@FeeJai
Copy link
Author

FeeJai commented Feb 21, 2015

Hey, thanks for your comments. I'm going to make a test case tomorrow - this problem occurs when there are manual validations like in the dm documentation.
I bumped the version number so that bundler would update and I agree that I should have done that in a separate branch. Sorry for including this into the PR

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.

3 participants