Skip to content

Print original exception after failing to raise it#9220

Merged
waj merged 2 commits intocrystal-lang:masterfrom
jhass:always_print_exception
Jul 2, 2020
Merged

Print original exception after failing to raise it#9220
waj merged 2 commits intocrystal-lang:masterfrom
jhass:always_print_exception

Conversation

@jhass
Copy link
Member

@jhass jhass commented May 3, 2020

No description provided.

@jhass jhass force-pushed the always_print_exception branch from ee41616 to 65eb39c Compare May 3, 2020 11:47
@jhass
Copy link
Member Author

jhass commented May 3, 2020

Well, unfortunately that was too easy. It seems to break CI, though locally the specs pass for me :(

@jhass jhass added the help wanted This issue is generally accepted and needs someone to pick it up label May 11, 2020
@jhass jhass force-pushed the always_print_exception branch from 65eb39c to 17e6387 Compare June 4, 2020 23:39
@jhass jhass removed the help wanted This issue is generally accepted and needs someone to pick it up label Jun 5, 2020
@jhass jhass requested a review from waj June 5, 2020 00:23
@jhass
Copy link
Member Author

jhass commented Jun 5, 2020

Well, looks like I fixed it!

@jhass jhass mentioned this pull request Jun 18, 2020
5 tasks
@waj
Copy link
Member

waj commented Jul 2, 2020

@jhass, this looks good, but is that rescue nil really necessary?

@jhass
Copy link
Member Author

jhass commented Jul 2, 2020

Probably not, it's admittedly defensive. I just figured given we're already in a weird state, we don't need to confuse users with any potential more weird error messages they cannot do anything about. But I don't care much about it, I can remove it if you prefer.

@waj
Copy link
Member

waj commented Jul 2, 2020

Yes, I understand. print_exception is actually quite defensive already 🙂 https://github.com/crystal-lang/crystal/blob/master/src/crystal/system/print_error.cr#L26

If you don't mind, I would remove that. Actually there are still many rescue nil in the std that I want to get rid of (and after that I would like to remove that feature completely 😝)

Copy link
Member

@waj waj left a comment

Choose a reason for hiding this comment

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

Thanks!

@jhass jhass added this to the 1.0.0 milestone Jul 2, 2020
@waj waj merged commit 586d7c3 into crystal-lang:master Jul 2, 2020
@jhass jhass deleted the always_print_exception branch July 3, 2020 07:57
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.

3 participants