-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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
Fixed a bug where AR::RecordNotSaved loses the given error message #17808
Fixed a bug where AR::RecordNotSaved loses the given error message #17808
Conversation
Since 3e30c5d, it started ignoring the given error message. This commit changes the behavior of AR::RecordNotSaved#initialize so that it no longer loses the given error message.
yeah.. this make sense 👍 |
@@ -54,9 +54,9 @@ class RecordNotFound < ActiveRecordError | |||
class RecordNotSaved < ActiveRecordError | |||
attr_reader :record | |||
|
|||
def initialize(record) | |||
def initialize(message, record = nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any place we don't pass record?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just saw that there are. I think we should always pass record
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that there are several questions around whether or not we should always include record
:
- Sometimes,
AR::RecordNotSaved
is raised before initializing an AR object. Do we need to initialize and passrecord
even in such a case? record
could be plural if it's raised fromcollection_association.rb
. Is it ok forrecord
to be plural at times? Also, do we want to include all the target records or only unsaved records?
I guess we should focus on a bug fix in this pull request as @arthurnn suggested earlier, and we can discuss the questions above after that.
@arthurnn @rafaelfranca what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I guess it is fine as it is now.
…loses-error-message Fixed a bug where AR::RecordNotSaved loses the given error message
This was extracted from #17806 that had an improvement and a bug fix. This is the bug fix.
Since 3e30c5d,
AR::RecordNotSaved
started ignoring the given error message. This commit changesAR::RecordNotSaved#initialize
to not ignore the given error message.Doing
create_or_update || raise(RecordNotSaved.new(nil, self))
inActiveRecord::Persistence
looks weird, but I'll send another pull request to improve it as soon as this one is merged.cc @arthurnn