Skip to content
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

More descriptive error messages #108

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

Sam-Robin
Copy link
Contributor

@Sam-Robin Sam-Robin commented Jul 24, 2024

While working with the discard gem I noticed that you can't discard already discarded records. There wasn't any mention of this in the docs and the error message wasn't very helpful. It cost me some time debugging.

Hopefully this saves others a bit of time :)

Comment on lines +680 to +684
it "raises Discard::RecordNotUndiscarded" do
expect(post).to receive(:do_before_undiscard) { abort_callback }
expect {
post.undiscard!
}.to raise_error(Discard::RecordNotUndiscarded)
}.to raise_error(Discard::RecordNotUndiscarded, "Failed to undiscard the record")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed this test had the wrong name

Copy link
Collaborator

@jarednorman jarednorman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this seems like a good change. I just need that typo fixed.

raise ::Discard::RecordNotUndiscarded.new(undiscarded_fail_message, self)
end

def disacrded_fail_message
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a typo in this method name.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@@ -677,11 +677,11 @@ def abort_callback
end

describe '#undiscard!' do
it "raises Discard::RecordNotDiscarded" do
it "raises Discard::RecordNotUndiscarded" do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

@jarednorman
Copy link
Collaborator

Looks good! I will merge this once I've figured out what's going on with CI.

@Sam-Robin
Copy link
Contributor Author

Thank you! Have a lovely day/afternoon/evening 😄

@jhawthorn jhawthorn merged commit b642d61 into jhawthorn:master Sep 10, 2024
@jhawthorn
Copy link
Owner

Thank you!

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