-
Notifications
You must be signed in to change notification settings - Fork 77
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
Fixing Attachment Error New Fork #14
Conversation
@@ -256,6 +257,9 @@ func TestAttachmentOnly(t *testing.T) { | |||
} | |||
|
|||
testMessage(t, m, 0, want) | |||
if err := teardownFile("/tmp/test.pdf"); err != nil { | |||
panic(err) |
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.
It's generally not a good practice to panic unless absolutely necessary. Have a look at using t.Errorf("teardown: %s", err)
instead.
@Gamma169 Thanks for making time for this over your weekend. On further consideration, I don't think we'll be able to benefit from this improvement without sacrificing usability/features somewhere else:
I was considering simply adding an |
@ivy why? this is a good patch, got a lot of stuff in there.. at least create the v3 branch.. |
@ivy serious do not reject.. accept everything as a possibility.. and id u do that.. then u have a whole heap behind u,, Thing u a good "leader" in making noise.. |
@ivy Understood about the I can think about things a little more and figure out a way to get rid of the bug (because I think it's pretty egregious). |
From PR go-gomail#97