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

raise(Exception): Setup callstack on raise only if not already set. #4487

Conversation

akzhan
Copy link
Contributor

@akzhan akzhan commented May 30, 2017

It's useful for reraising of exceptions in rescue blocks.

As proposed by #4482 (comment).

/cc @asterite, @straight-shoota.

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

Since the exception is raised from the same context (the spec block), aren't the callstacks equivalent?

src/raise.cr Outdated
# Raises the *exception*.
#
# Note that exception callstack will only be set if it's not already set.
# It's useful for reraising of exceptions in `rescue` blocks.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the doc could be simpler? Maybe:

Raises an Exception.

This will set the exception's callstack if it hasn't been already. Re-raising a previously catched exception won't replace the callstack.

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 suppose that the article just fit here.

And thanks for rewording, will apply.

new_ex.callstack.should eq(callstack_to_match)
end

it "should throw exception with new callstack if not already set" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it sets exception's callstack and it should come before the spec that says it doesn't overwrite the callback.

It's useful for reraising of exceptions in `rescue` blocks.

As proposed by crystal-lang#4482 (comment).

/cc @asterite, @straight-shoota.
@akzhan akzhan force-pushed the setup-callstack-on-raise-only-if-not-already-set branch from b357676 to 82eb2d4 Compare May 30, 2017 15:15
@akzhan
Copy link
Contributor Author

akzhan commented May 30, 2017

@ysbaddaden I believe that we should not depend in this example on the assumption that we get the same exception.

@akzhan
Copy link
Contributor Author

akzhan commented May 30, 2017

Although you are right, this is absolutely superfluous

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

Structs don't need clone because they are value types. And their equals method by default compares all fields. Please remove the struct reopening.

Copy link
Member

@asterite asterite left a comment

Choose a reason for hiding this comment

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

The spec is also wrong. You should raise, rescue and re-raise in the spec. That's what we really want to happen.

@akzhan
Copy link
Contributor Author

akzhan commented May 30, 2017

Thanks, @asterite . Now it looks cleaner.

@ysbaddaden ysbaddaden merged commit 6b3bde1 into crystal-lang:master May 31, 2017
@ysbaddaden
Copy link
Contributor

Thank you!

@akzhan akzhan deleted the setup-callstack-on-raise-only-if-not-already-set branch May 31, 2017 14:31
@mverzilli mverzilli added this to the Next milestone Jun 2, 2017
@mverzilli
Copy link

@ysbaddaden would you mind setting the "Next" milestone to PRs when you merge them? It makes it easier for us to write release notes afterwards :)

@ysbaddaden
Copy link
Contributor

Oh ok, will do now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants