More refactorings #2
Conversation
It turns out, a global variable is not a great idea, so we move it into the Mail object itself. This is also introduces a new "chaninable" API: `m.SetSendmail()` returns `m`, which allows chaining of further options (see following commits).
Don't depend on environment variable for debug output, instead let the consumer decide where to print to.
AppendTo might be a bit misleading, as it doesn't append the mail to something, but appends the mail's To field. It is literally a `To = append(To, ...)` construct.
Accepts chainable Options as arguments.
Makes testing unneccessarily hard.
Place multipart mails (HTML+Text) on the roadmap.
sendmail.go
Outdated
| } | ||
|
|
||
| // WriteTo writes headers and content of the email to io.Writer | ||
| func (m *Mail) WriteTo(wr io.Writer) error { | ||
| func (m *Mail) WriteTo(wr io.Writer) error { // nolint: vet |
There was a problem hiding this comment.
If we are breaking compatibility with previous versions, then return values from this function can be also changed to these suggested by go vet.
There was a problem hiding this comment.
Yeah, I was about to ask about that, but forgot.
Since writing headers doesn't return the number of bytes written, the number we'd return could either be off by that amount, or always be 0. (We could also wrap the wr with something that counts the number of bytes written for us, but honestly, I don't think it matters that much...)
What do you think?
options.go
Outdated
| // AppendTo adds a recipient to the Mail. The name argument is the | ||
| // "proper name" of the recipient and may be empty. The address must be | ||
| // in the form "user@domain". | ||
| func (m *Mail) AppendTo(name, address string) *Mail { |
There was a problem hiding this comment.
Here we can use *mail.Address directly, it's a build in type. I can imagine situation when somebody used ParseAddress, which returns *mail.Address, then he need to convert it to 2 strings and we convert it back to *mail.Address. That would be a bit over engineered. Using mail.Address as a function parameter doesn't bring that much complexity I think: AppendTo(&mail.Address{Name: "someone", Address: "some@email"}). @dmke what do you think?
WriteTo now returns (int64, error); however the returned number of bytes written is always 0, since we don't get the actual number easily.
|
I've addressed your comments and added some commits. I will now try to fix the issue of |
|
FYI: I've worked on this on company time, so I need to move the code to my emplyers repository. This may require a new PR, depending on how Github handles moving repos :-) |
|
Any chance to get this merged in? |
Hey,
whilst using this more, I found I needed a more conveniant interface.
The first part in this commit series will introduce a
sendmail.Optiontype and asendmail.New()constructor function, which takes said Options as arguments. Together, they allow code to look like this:or like this:
Another subset of the commits moves the recently introduced global
Binaryvariable, as well as the debug variable into theMailtype. As it turns out, global variables are a source of frustration in concurrent applications :-) You can see an example in the snippets above (SetOutput/SetSendmail).I also started with HTML mails, but mixed-content (multipart, Text+HTML) isn't there yet. I'd like to harvest some code from mailyak, but the current license prevents me from that. Having a solid MIME multipart implementation would also allow attachments to the mail.