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

Fixing Attachment Error New Fork #14

Closed
wants to merge 15 commits into from
Closed

Fixing Attachment Error New Fork #14

wants to merge 15 commits into from

Conversation

Gamma169
Copy link

From PR go-gomail#97

@@ -256,6 +257,9 @@ func TestAttachmentOnly(t *testing.T) {
}

testMessage(t, m, 0, want)
if err := teardownFile("/tmp/test.pdf"); err != nil {
panic(err)
Copy link

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.

@ivy
Copy link

ivy commented Dec 10, 2017

@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:

  • SetCopyFunc allows replacing the underlying file-backed copier with anything that acts on an io.Writer (as you see in the mocked tests). We need something that still allows this functionality as it's useful in other areas outside of testing such as in Accept io.Reader for Attach and Embed go-gomail/gomail#78.
  • Rename makes things trickier as we also can't rely on the filename matching what's on disk.

I was considering simply adding an error return value to Embed and Attach but that also breaks SetCopyFunc for non-file-backed attachments. To sum things up, we're going to have to wait on this until there's a solid plan for v3.

@ivy ivy closed this Dec 10, 2017
@pedromorgan
Copy link

@ivy why?

this is a good patch, got a lot of stuff in there..

at least create the v3 branch..

@pedromorgan
Copy link

@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..
But this is code ok...
and its differrent kinda respect.....and trust

@Gamma169
Copy link
Author

@ivy Understood about the SetCopyFunc function. @pedromorgan The problem is with that function, a person can attach data and label it as a file that doesn't exist. Thus triggering the error on the send, when there actually is no file.

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).

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