-
Notifications
You must be signed in to change notification settings - Fork 184
Mimics ActiveRecord behavior when destroying resource #314
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
Conversation
* Add `destroyed?` method * Return false on `persisted?` * Do not clear resource attributes Co-authored-by: Gabriel Reis <gabriel@hashrocket.com>
lib/json_api_client/resource.rb
Outdated
| else | ||
| self.attributes.clear | ||
| mark_as_destroyed! | ||
| self.relationships.attributes.clear |
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.
if we don't want to clear attributes after destroy, why we should clear relationships?
Co-authored-by: Mary Lee <mary.lee@hashrocket.com> Co-authored-by: Gabriel Reis <gabriel@hashrocket.com>
gaorlov
left a comment
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.
That looks great! Thanks a ton for the PR and the work.
| user = users.first | ||
| assert(user.persisted?) | ||
| assert_equal(false, user.new_record?) | ||
| assert_equal(false, user.destroyed?) |
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.
You can also do refute user.new_record?, but it really doesn't matter
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.
We can change it if you'd like; we were just trying to follow the style of the existing tests. Do you want us to update?
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.
Hi 👋, recently I played with Rubocop. It suggests to use ActiveSupport method assert_not
assert_not user.new_record?PS: more than one way of doing the same thing 😏 Its up to commits author, what to use.
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.
@hashrocketeer, nah. Thanks again for the contribution! I'll push out a new version with your code by eod.
|
@hashrocketeer |
destroyed?methodpersisted?after being destroyednew_record?after being destroyed