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

GSoC: finagle-smtp - initial #287

Closed
wants to merge 50 commits into from
Closed
Changes from 45 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
27a48e6
A quick&dirty example of SMTP client
suncelesta Mar 12, 2014
7de8295
simple wrappers for javamail and a.c.e.
suncelesta May 6, 2014
d8f4877
New client API
suncelesta May 7, 2014
3684bf5
SMTP commands
suncelesta May 16, 2014
18f43f2
Reply classes
suncelesta May 26, 2014
728d7f9
Dispatcher and transport
suncelesta Jun 3, 2014
5b9e617
Finished codec
suncelesta Jun 9, 2014
507a775
Filters concerning email payload
suncelesta Jun 10, 2014
4410eeb
Receiving server greeting before the session
suncelesta Jun 11, 2014
f5e7604
Reply code refactoring
suncelesta Jun 11, 2014
bed4e94
Minor bugs fix
suncelesta Jun 11, 2014
699d78d
More concise result in simple client
suncelesta Jun 11, 2014
067917c
Full multiline support
suncelesta Jun 11, 2014
14adab8
Refined structure, tests for filters
suncelesta Jun 17, 2014
69aec96
More tests
suncelesta Jun 21, 2014
3664948
Multiline correction and usage doc
suncelesta Jun 24, 2014
8614e7a
Update README.md
selvin Jun 24, 2014
017d516
Update README.md
selvin Jun 24, 2014
074d72d
Update README.md
selvin Jun 24, 2014
a187f29
Update README.md
selvin Jun 24, 2014
571e85a
Update README.md
selvin Jun 24, 2014
41f35c8
Update README.md
selvin Jun 24, 2014
8626554
Merge pull request #1 from selvin/master
suncelesta Jun 25, 2014
f6c8be9
Sending quit command upon service closing
suncelesta Jun 25, 2014
358655c
Fixed issue with build error
suncelesta Jun 25, 2014
adfc22d
EHLO sends domain/address literal
suncelesta Jul 3, 2014
cd9efe4
Copies and EmailBuilder
suncelesta Jul 5, 2014
3ea84a4
Logs
suncelesta Jul 5, 2014
53c9b63
Tests for MailAddress and EmailBuilder
suncelesta Jul 5, 2014
0ea3914
SmtpSimple sends EHLO once in the beginning
suncelesta Jul 7, 2014
597a8c8
..
suncelesta Jul 7, 2014
bc1ac67
resolve conflict
suncelesta Jul 7, 2014
bd2e836
delete unnecessary file
suncelesta Jul 7, 2014
e560f60
Fix accidental mysql test rearrangement
suncelesta Jul 9, 2014
275ec98
Moved Example.scala to finagle-example
suncelesta Jul 9, 2014
7fa4157
Removed left diff on mysql RequestTest
suncelesta Jul 10, 2014
5681625
try to remove end of line diff
suncelesta Jul 10, 2014
ed5ec40
Minor fixes
suncelesta Jul 10, 2014
bd15c38
Merge branch 'finagle-smtp' of https://github.com/suncelesta/finagle …
suncelesta Jul 10, 2014
258be78
Fixed Sender field
suncelesta Jul 11, 2014
178ae73
Fixed README.MD
suncelesta Jul 11, 2014
f2f6a7e
Added scaladoc comments
suncelesta Jul 17, 2014
9b3c003
Added link to RFC
suncelesta Jul 23, 2014
c04fca9
Corrected link to Example in README
suncelesta Jul 28, 2014
a1f96a8
Email headers
suncelesta Aug 1, 2014
78fe18d
Fixed style and logic
suncelesta Aug 8, 2014
a342264
Field names converted to lowercase
suncelesta Aug 11, 2014
caa31e3
DefaultEmail instead of EmailBuilder
suncelesta Aug 11, 2014
8adbf4d
Quit moved to dispatcher
suncelesta Aug 14, 2014
03d9be6
Fixes for all tests to pass
suncelesta Aug 17, 2014
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -0,0 +1,42 @@
package com.twitter.finagle.example.smtp

import com.twitter.util.{Await, Future}

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

sort imports alphabetically please (within the same line it's OK to be unordered)

import com.twitter.finagle.smtp._
import com.twitter.finagle.SmtpSimple
import com.twitter.logging.Logger

/**
* Simple SMTP client with an example of error handling.
*/
object Example {
private val log = Logger.get(getClass)

def main(args: Array[String]) = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Please add an explicit return type annotation. All public methods should have explicit return types. In this case, Unit.

// Raw text email
val email = EmailBuilder()
.sender("from@from.com")

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

we don't use this style elsewhere in finagle, please just indent by two spaces rather than aligning.

.to("first@to.com", "second@to.com")
.subject("test")
.addBodyLines("first line", "second line") //body is a sequence of lines
.build

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

in general, we use 0-arity methods without parentheses if it acts like a field. build() doesn't act anything like a field, so let's use parens.


// Connect to a local SMTP server
val send = SmtpSimple.newService("localhost:25")

// Send email
val res: Future[Unit] = send(email)
.onFailure {
// An error group

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

please indent two spaces in these kinds of blocks.

case ex: reply.SyntaxErrorReply => log.error("Syntax error: ", ex.info)

// A concrete reply
case reply.ProcessingError(info) => log.error("Error processing request: ", info)
}

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

this should match the indentation level of the line that started the block.


println("Sending email...") // This will be printed before the future returns

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

log these instead of just sending them to stdout.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

also there isn't a guarantee that it will be printed before the future returns, since it might buffer.


Await.ready(res)

This comment has been minimized.

Copy link
@selvin

selvin Jul 29, 2014

isn't it necessary to block? otherwise this thread ends and since all other threads are daemon threads, the program exits shortly thereafter

This comment has been minimized.

Copy link
@suncelesta

suncelesta Jul 31, 2014

Author

That was what I meant, I just supposed that in practice the program would probably not end like this, but have something else done in background.


println("Sent")
}
}
@@ -0,0 +1,67 @@
# finagle-smtp

A minimum implementation of SMTP client for finagle according to [`RFC5321`](http://tools.ietf.org/search/rfc5321).

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Please make all of these line lengths < 100 characters.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Is this RFC the best resource to learn about smtp? Might be useful to include general resources to help n00bs get started.

Supports sending plain text emails to one or more mailing addresses.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Why only plaintext? What kinds of restrictions are there?


## Usage

### Sending an email

The object for instantiating a client capable of sending a simple email is `SmtpSimple`.
For services created with it the request type is `EmailMessage`, described in `EmailMessage.scala`.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add a link to EmailMessage.scala.


You can create an email using `EmailBuilder` class described in `EmailBuilder.scala`:

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add a link.


```scala
val email = EmailBuilder()
.sender("from@from.com")

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

fix indentation to just two spaces, instead of aligned. should look like:

val email = EmailBuilder()
  .sender....
.to("first@to.com", "second@to.com")
.subject("test")
.bodyLines("first line", "second line") //body is a sequence of lines
.build
```

Applying the service on the email returns `Future.Done` in case of a successful operation or the first encountered error.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Why does it return Future.Done on an error? Wouldn't you expect it to return a failed Future?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

It doesn't. It returns Future.done in case of success. In case of failure it returns the first encountered error wrapped in a Future.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

OK, the docs make it sound like it returns Future.Done regardless. Can you fix the docs?


#### Greeting and session

Upon the connection the client receives server greeting.
In the beginning of the session an EHLO request is sent automatically to identify the client.
The session state is reset before every next try.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

"every next try" => "every subsequent try".


### Example

The example of sending email to a local SMTP server with SmtpSimple and handling errors can be seen in [`Example.scala`](https://github.com/suncelesta/finagle/blob/finagle-smtp/finagle-example/src/main/scala/com/twitter/finagle/example/smtp/Example.scala).

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

It's a little messy to use the inline linking, might be easier to read the raw file if you use the [0] linking instead.


### Sending independent SMTP commands

The object for instantiating an SMTP client capable of sending any command defined in *RFC5321* is `Smtp`. For services created with it the request type is `Request`.
Command classes are described in `Request.scala`. Replies are differentiated by groups, which are described in `reply/ReplyGroups.scala`.
The concrete reply types are case classes described in `reply/SmtpReplies.scala`.

This allows flexible error handling:

```scala
val res = service(command) onFailure {
// An error group
case ex: SyntaxErrorReply => log.error("Syntax error: ", ex.info)
// A concrete reply
case ProcessingError(info) => log,error("Error processing request: ", info)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

My impression of these styles of logging is that you need to format the string to be ready to get arguments, à la "Error processing request: %s", info.

// Default
case _ => log,error("Error!")

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

I think this should be log.error. ditto a few lines up.

}
// Or, another way:
res handle {
...
}
```

#### Greeting and session

Default SMTP client only connects to the server and receives its greeting, but does not send greeting to it,

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

"but does not send greeting to it" => "but does not return a greeting"
"without that" => "without it".

By incorrect, do you mean malformed?

as some commands may be executed without that. If the greeting is incorrect, the service is closed.
Upon closing the service a quit command is sent automatically, if not sent earlier.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

What kind of closing? If I send a kill -9, presumably it won't send a quit command.

@@ -0,0 +1,82 @@
package com.twitter.finagle

import com.twitter.util.{Time, Future}

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

sort imports

import com.twitter.finagle.client.{DefaultClient, Bridge}
import com.twitter.finagle.smtp._
import com.twitter.finagle.smtp.reply._
import com.twitter.finagle.smtp.filter.{MailFilter, HeadersFilter, DataFilter}
import com.twitter.finagle.smtp.transport.SmtpTransporter


/**
* Implements an SMTP client in terms of a finagle DefaultClient.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

we don't need to know it's implemented with a DefaultClient, it's an implementation detail.

* This type of client is capable of sending independent

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

this comment probably goes without saying.

* SMTP commands and receiving replies to them.
*/
object Smtp extends Client[Request, Reply]{

val defaultClient = DefaultClient[Request, Reply] (

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

The way this works has changed slightly--right now, StackClient is the new way of doing this. I know you're near the end of your summer, so you might not have time to change this, but might be worth looking and seeing if switching over is simple.

name = "smtp",
endpointer = {
val bridge = Bridge[Request, UnspecifiedReply, Request, Reply](
SmtpTransporter, new SmtpClientDispatcher(_)
)
(addr, stats) => bridge(addr, stats)
})

/**
* Constructs an SMTP client.
*
* Upon closing the connection this client sends QUIT command;
* it also performs dot stuffing.
*/
override def newClient(dest: Name, label: String) = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type


val quitOnCloseClient = new ServiceFactoryProxy[Request, Reply](defaultClient.newClient(dest, label)){

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add a space before the {, this line is > 100 characters.


override def apply(conn: ClientConnection) = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type

self.apply(conn) flatMap { service =>

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

instead of flatMap { ... Future.value(x) } just do map { ... x }

val quitOnClose = new ServiceProxy[Request, Reply](service) {
override def close(deadline: Time) = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type

if (service.isAvailable)
service(Request.Quit)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

is it OK to send many of these? it seems like it would be easy for many threads to try to close this service many times.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

I'm not sure this is the right layer to have this at. Maybe it would make more sense in your dispatcher? Then you wouldn't entail a new allocation every time you requested a new connection.

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 8, 2014

Author

The problem is, when I put this in the dispatcher, nothing is actually sent at the end of the session. And the quit request must be sent before closing the connection.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 8, 2014

Contributor

So I think by default, finagle will hold open a tcp connection until you actually shut down the client. From what you said about the length of smtp sessions, it sounds like that's correct behavior, no? Or is the problem that you are destroying the client and not seeing the request be sent? We also might have to sequence service(Request.Quit) and service.close(deadline).

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 8, 2014

Author

Yes, when testing with SMTP server stub and looking at its logs, I found out that this request is not actually sent from dispatcher, so I sought the way to enforce service(Request.Quit) before service.close(deadline).

service.close(deadline)
}
}
Future.value(quitOnClose)
}
}
}

DataFilter andThen quitOnCloseClient
}
}

/**
* Implements an SMTP client that can send an [[com.twitter.finagle.smtp.EmailMessage]].
* The application of this client's service doesn't return a

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

We don't need to describe what it doesn't do, describing what it does is sufficient. However, for my edification, could you explain how you chose to just return a Future.Done? Is the reply in the smtp spec anemic?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

The logic behind this is such: you either send the email successfully, or there is some error that prevents it to be sent. The request type here is an email, not an SMTP command. Which of the many replies SMTP server sent during the mail session should be chosen to represent success in this case? Furthermore, even if you take 250 reply when mail data is accepted, it's still the only one to be expected in case of success, so is there any point in keeping that information?

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

Why does smtp have so many different replies? Or does it only have one success reply per command? Do replies add granularity to success?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 8, 2014

Author

The success replies to different commands usually have different informational strings, so for each command there is actually a specific success reply. Not all of them even have the same reply code. However, the email can be sent only if all the commands succeed. (Here, if one of the recepients is not accepted, the session is aborted, and the error pertained to that recipient is returned; for more flexibility there is Smtp client).

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 8, 2014

Contributor

Got it. Seems like it would be useful for implementors, but not useful for users of a client library.

* reply; instead, it returns [[Future.Done]] in case of success
* or the first encountered error in case of a failure.
*/
object SmtpSimple extends Client[EmailMessage, Unit] {
/**
* Constructs an SMTP client that sends a hello request
* in the beginning of the session to identify itself;
* it also copies email headers into the body of the message.
* The dot stuffing and connection closing
* behaviour is the same as in [[Smtp.newClient()]].
*/
override def newClient(dest: Name, label: String) = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type annotation

val startHelloClient = new ServiceFactoryProxy[Request, Reply](Smtp.newClient(dest, label)) {
override def apply(conn: ClientConnection) = {
self.apply(conn) flatMap { service =>

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

map instead of flatMap

service(Request.Hello)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

not sure this is in the right location. I don't know SMTP that well, is there any reason why we wouldn't want to start a connection with this? maybe it should just be in the dispatcher?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

A hello request identifies the client, and the RFC recommends that SMTP sessions are started with it. It can be moved to the connection phase in the dispatcher, though.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

Hurk, how long do SMTP sessions last? Do we start an SMTP session with a tcp connection and tear it down when we turn down the SMTP session, or do we set it up / tear it down with the request?

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 8, 2014

Author

The first case. The session lasts until quit command is sent (and then the server closes the connection) or some connection error occurs.

Future.value(service)
}
}
}
HeadersFilter andThen MailFilter andThen startHelloClient
}
}


@@ -0,0 +1,190 @@
package com.twitter.finagle.smtp

import java.util.{Calendar, Date}

/** The payload of an email. */
case class Payload(from: Seq[String],

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

indentation, switch over to two spaces. Also, put the from: Seq[String] on the next line.

sender: String,
to: Seq[String],
cc: Seq[String],
bcc: Seq[String],
reply_to: Seq[String],

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

please switch from snake_case to camelCase.

date: String,
subject: String,
body: Seq[String])

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

put the end paren on the next line.



/**
* Composes an [[com.twitter.finagle.smtp.EmailMessage]]. */

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

*/ should be on the next line.

"Constructs" might be more clear than "composes".

case class EmailBuilder(payload: Payload) {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

Let's collapse EmailBuilder and Payload. I don't see a clear benefit here to separating our data and our behavior.

/**
* Adds originator addresses, which will appear in the 'From:' field.
*
* @param addrs Addresses to be added
*/
def from(addrs: String*): EmailBuilder = setFrom(payload.from ++ addrs)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

looks like we don't do any validation on email addresses. this is probably worth documenting somewhere, or else adding a TODO.

This comment has been minimized.

Copy link
@suncelesta

suncelesta Aug 7, 2014

Author

If you mean that email addresses should be checked to look like user@domain.com, this is done when constructing MailingAddress'es from them. I can add this type of requirement here too.

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 7, 2014

Contributor

No need, just document the contract.


/**
* Set the ''From:'' field to contain given originator addresses.
*
* @param addrs Addresses to be set
*/
def setFrom(addrs: Seq[String]): EmailBuilder = copy(payload.copy(from = addrs))

/**
* Sets the ''Sender:'' field to contain given mailing address.
* Sender should be specified if there are multiple
* originator addresses.
* If the given address is not included in ''From:'' field,
* it is added there.
*
* @param addr Address to be set
*/
def sender(addr: String): EmailBuilder = {
if(payload.from contains addr) copy(payload.copy(sender = addr))
else copy(payload.copy(sender = addr, from = payload.from :+ addr))
}

/**
* Adds recipient addresses, which will appear in the ''To:'' field.
*
* @param addrs Addresses to be added
*/
def to(addrs: String*): EmailBuilder = setTo(payload.to ++ addrs)

/**
* Set the ''To:'' field to contain given recipient addresses.
*
* @param addrs Addresses to be set
*/
def setTo(addrs: Seq[String]): EmailBuilder = copy(payload.copy(to = addrs))

/**
* Adds carbon copy addresses, which will appear in the ''Cc:'' field.
*
* @param addrs Addresses to be added
*/
def cc(addrs: String*): EmailBuilder = setCc(payload.cc ++ addrs)

/**
* Set the ''Cc:'' field to contain given carbon copy addresses.
*
* @param addrs Addresses to be set
*/
def setCc(addrs: Seq[String]): EmailBuilder = copy(payload.copy(cc = addrs))

/**
* Adds blind carbon copy addresses, which will appear in the ''Bcc:'' field.
*
* @param addrs Addresses to be added
*/
def bcc(addrs: String*): EmailBuilder = setBcc(payload.bcc ++ addrs)

/**
* Set the ''Bcc:'' field to contain given blind carbon copy addresses.
*
* @param addrs Addresses to be set
*/
def setBcc(addrs: Seq[String]): EmailBuilder = copy(payload.copy(bcc = addrs))

/**
* Adds the addresses to reply to, which will appear in the ''Reply-To:'' field.
*
* @param addrs Addresses to be added
*/
def reply_to(addrs: String*): EmailBuilder = setReplyTo(payload.reply_to ++ addrs)

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

camelCase instead of snake_case.


/**
* Set the ''Reply-To:'' field to contain given addresses to reply to.
*
* @param addrs Addresses to be set
*/
def setReplyTo(addrs: Seq[String]): EmailBuilder = copy(payload.copy(reply_to = addrs))

/**
* Set the date of sending the message.
*
* @param dt Date to be set
*/
def date(dt: Date): EmailBuilder = copy(payload.copy(date = EmailMessage.DateFormat.format(dt)))

/**
* Set the subject of the message.
*
* @param sbj Subject to be set
*/
def subject(sbj: String): EmailBuilder = copy(payload.copy(subject = sbj))

/**
* Add lines to the message text.
*
* @param lines Lines to be added
*/
def addBodyLines(lines: String*): EmailBuilder = setBodyLines(payload.body ++ lines)

/**
* Add lines to the message text.
*
* @param lines Lines to be added
*/
def setBodyLines(lines: Seq[String]): EmailBuilder = copy(payload.copy(body = lines))

/**
* Instantiate an [[com.twitter.finagle.smtp.EmailMessage]] from the payload.
* If the date of sending the message is not set,
* current date is used. If sender of the message
* is not specified, the first address in ''From:'' is used.
*/
def build: EmailMessage = new EmailMessage {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add parens

def headers = {

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type annotation

payload.from.map(("From",_)) ++
Seq("Sender" -> { if (payload.sender.nonEmpty) payload.sender
else payload.from.head }) ++
payload.to.map(("To",_)) ++
payload.cc.map(("Cc",_)) ++
payload.bcc.map(("Bcc",_)) ++
payload.reply_to.map(("Reply-To",_)) ++
Seq(
"Date" -> { if (payload.date != null) payload.date

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

we don't assume anything is null by default, why do we assume date might be null? if it's OK to not set it, let's make it an Option.

else EmailMessage.currentTime },
"Subject" -> payload.subject
)
}

def body = payload.body

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type annotation

}

}

/**
* Factory for [[com.twitter.finagle.smtp.EmailBuilder]] instances
*/
object EmailBuilder {
/**
* Creates a default EmailBuilder with empty payload.
*/
def apply() = new EmailBuilder(Payload(from = Seq.empty,

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

return type annotation

sender = "",

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

two spaces indentation

to = Seq.empty,
cc = Seq.empty,
bcc = Seq.empty,
reply_to = Seq.empty,
date = null,

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

in idiomatic scala, we usually use Options for optional arguments.

subject = "",
body = Seq.empty))

/**
* Creates an [[com.twitter.finagle.smtp.EmailBuilder]] with payload from given [[com.twitter.finagle.smtp.EmailMessage]].

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

line length

*
* @param msg The message to copy payload from
*/
def apply(msg: EmailMessage) = new EmailBuilder(Payload(from = msg.from.map(_.mailbox),

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

add return type

sender = msg.sender.mailbox,

This comment has been minimized.

Copy link
@mosesn

mosesn Aug 6, 2014

Contributor

fix indentation

to = msg.to.map(_.mailbox),
cc = msg.cc.map(_.mailbox),
bcc = msg.bcc.map(_.mailbox),
reply_to = msg.replyTo.map(_.mailbox),
date = EmailMessage.DateFormat.format(msg.date),
subject = msg.subject,
body = msg.body))
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.