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

Remove backtrace from ECR errors #4851

Merged
merged 1 commit into from Sep 16, 2017
Merged

Conversation

bew
Copy link
Contributor

@bew bew commented Aug 18, 2017

Beautify ECR errors for issues like #4577, by removing the stacktrace.

Before:
before
After:
after

@straight-shoota
Copy link
Member

Maybe add a comment to explain the purpose of the rescue?

@bew bew force-pushed the clean-ecr-error branch 2 times, most recently from b7fafc6 to 007c23c Compare August 19, 2017 11:21
@bew
Copy link
Contributor Author

bew commented Aug 19, 2017

I've restricted the rescue, to rescue only Errno errors, so other errors will have their backtrace for debugging if needed.

@bew bew changed the title Beautify ECR errors Remove backtrace from ECR errors Aug 20, 2017
@RX14
Copy link
Contributor

RX14 commented Aug 20, 2017

I think we should only rescue some Errno's, specifically ENOENT.

@bew
Copy link
Contributor Author

bew commented Aug 20, 2017

@RX14 done. I rescued ENOENT & EISDIR, I think thoses two will be the most common in this case.

begin
puts ECR.process_file(filename, buffer_name)
rescue ex : Errno
raise ex unless {Errno::ENOENT, Errno::EISDIR}.includes?(ex.errno)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather reverse this:

if {Errno::ENOENT, Errno::EISDIR}.includes? ex.errno
  STDERR.puts ex.message
  exit 1
else
  raise ex
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?
In my coding-style I tend to discard "invalid" values (early returns), then have the content of what want to do in the method/rescue/anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense when the method body is long, but here's it's just two statements, making the if clearer. Once it gets to 5-7+ lines i'd agree with you.

Copy link
Member

Choose a reason for hiding this comment

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

This is a rescue block, it shouldn't get that long... ;)

Rescure only Errno::ENOENT & Errno::EISDIR errors, as thoses are
the most common errors in this case.
@bew
Copy link
Contributor Author

bew commented Aug 29, 2017

@RX14 I've updated to use long-form if in the rescue block.

@Sija
Copy link
Contributor

Sija commented Sep 10, 2017

Ready to 🚢

@mverzilli mverzilli added this to the Next milestone Sep 16, 2017
@mverzilli mverzilli merged commit af8c4f6 into crystal-lang:master Sep 16, 2017
@bew bew deleted the clean-ecr-error branch September 17, 2017 00:39
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

5 participants