PTAL
https://codereview.appspot.com/91000044/diff/1/README.md
File README.md (right):
https://codereview.appspot.com/91000044/diff/1/README.md#newcode4
README.md:4: TweetSnip is a sample application showing how to use the mail
service provided by
On 2014年05月05日 00:49:00, adg wrote:
> s/showing/that shows/
Done.
https://codereview.appspot.com/91000044/diff/1/app/app.yaml
File app/app.yaml (right):
https://codereview.appspot.com/91000044/diff/1/app/app.yaml#newcode13
app/app.yaml:13: - url: /_ah/mail/(subscribe|unsubscribe|tweet)@.*
On 2014年05月05日 00:49:00, adg wrote:
> why not just use "/_ah/mail/.*" and have mailmux send the 404?
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go
File app/tweetsnip.go (right):
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode13
app/tweetsnip.go:13: // To se the period length read the cron.yaml file.
On 2014年05月05日 00:49:02, adg wrote:
> set
> s/length //
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode33
app/tweetsnip.go:33: gaemail "appengine/mail"
On 2014年05月05日 00:49:02, adg wrote:
> the common renaming is "netmail" for "net/mail" and leaving "appengine/mail"
as
> "mail"
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode47
app/tweetsnip.go:47: // A Snippet groups a list of Tweets that are sent via
mail.
On 2014年05月05日 00:49:00, adg wrote:
> Snippet is a bad name for this; it is singular, and is not a collective noun.
> why not just "Group"
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode60
app/tweetsnip.go:60: func LatestSnippet(ctx appengine.Context) (*Snippet, error)
{
On 2014年05月05日 00:49:01, adg wrote:
> I would tend not to export these handlers, as they would have no practical use
> outside the package. The Snippet type, however, should stay exported.
I decided to export so they will appear in godoc
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode69
app/tweetsnip.go:69: p := &Snippet{1}
On 2014年05月05日 00:49:01, adg wrote:
> add a comment:
> 
> // Create a snippet if it does not exist.
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode77
app/tweetsnip.go:77: // tweetsInSnippet returns all the tweets in the given
snippet.
On 2014年05月05日 00:49:01, adg wrote:
> comment doesn't match function name
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode82
app/tweetsnip.go:82: return nil, fmt.Errorf("get unseen snippets: %v", err)
On 2014年05月05日 00:49:00, adg wrote:
> what's the meaning of this error message?
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode88
app/tweetsnip.go:88: func (p *Snippet) Put(ctx appengine.Context) error {
On 2014年05月05日 00:49:01, adg wrote:
> this method doesn't really add anything
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode99
app/tweetsnip.go:99: Content string // Content of the tweet.
On 2014年05月05日 00:49:02, adg wrote:
> "Tweets" are supposed to be short, but this will be hard limited to 255 unless
> you add a struct tag: `datastore:",noindex"`
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode118
app/tweetsnip.go:118: // All snippets share a dumb ancestor to be able to query
them during transactions.
On 2014年05月05日 00:49:01, adg wrote:
> "dumb" can be considered derogatory toward people who cannot talk.
> "dummy" is probably the word you're looking for, but "root" is better in this
> context.
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode123
app/tweetsnip.go:123: // subscriptions returns the list of active subscriptions.
On 2014年05月05日 00:49:01, adg wrote:
> doesn't it return a list of active subscriber emails?
> 
> userEmails? 
> 
> and what does "active" mean? There's no such thing as an "inactive" user in
this
> system;
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode125
app/tweetsnip.go:125: var usrs []User
On 2014年05月05日 00:49:01, adg wrote:
> s/usrs/users/
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode131
app/tweetsnip.go:131: to := make([]string, 0, len(usrs))
On 2014年05月05日 00:49:01, adg wrote:
> don't preallocate the array. this happens twice a week
> 
> var to []string
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode132
app/tweetsnip.go:132: for _, usr := range usrs {
On 2014年05月05日 00:49:00, adg wrote:
> s/usr/u/
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode133
app/tweetsnip.go:133: to = append(to, usr.Email)
On 2014年05月05日 00:49:02, adg wrote:
> if you _do_ preallocate an array (and you needn't here), you should use
> 
> for i, u := range users { 
> to[i] = u.Email
> }
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode157
app/tweetsnip.go:157: ctx.Infof("no active users, skipping email")
On 2014年05月05日 00:49:00, adg wrote:
> s/active //
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode171
app/tweetsnip.go:171: goodGuys := make(map[string]bool)
On 2014年05月05日 00:49:01, adg wrote:
> goodguys badguys?
> cute but bad for readability
> 
> "seen" and "to"
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode188
app/tweetsnip.go:188: "So what's awesome?",
On 2014年05月05日 00:49:01, adg wrote:
> make these string constants at the top of the file
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode193
app/tweetsnip.go:193: Last week some cool things happened!
On 2014年05月05日 00:49:01, adg wrote:
> "Here's what happened last week:" ?
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode199
app/tweetsnip.go:199: // SendSnippets sends an email to all subscribed users
with all the submitted tweets that
On 2014年05月05日 00:49:01, adg wrote:
> wrap these comments to 80 columns
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode207
app/tweetsnip.go:207: ctx.Infof("no active users, skipping snippets")
On 2014年05月05日 00:49:02, adg wrote:
> s/active //
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode231
app/tweetsnip.go:231: err = send(ctx, to, "The awesome things gophers did last
week", buf.String())
On 2014年05月05日 00:49:02, adg wrote:
> IIRC emails aren't considered part of a transaction, so you should send the
mail
> once the transaction has completed successfully.
> 
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode240
app/tweetsnip.go:240: // readBody reads the body of the given message, handling
the multipart format.
On 2014年05月05日 00:49:01, adg wrote:
> readBody returns the plain text component of the provided message.
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode276
app/tweetsnip.go:276: s, e := strings.Index(res, "<"), strings.Index(res, ">")
On 2014年05月05日 00:49:01, adg wrote:
> there's a function in net/mail to parse these lines
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode284
app/tweetsnip.go:284: func SubscribeUser(ctx appengine.Context, m *mail.Message)
error {
On 2014年05月05日 00:49:01, adg wrote:
> subscribeHandler
> or
> handleSubscribe
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode292
app/tweetsnip.go:292: func UnsubscribeUser(ctx appengine.Context, m
*mail.Message) error {
On 2014年05月05日 00:49:02, adg wrote:
> unsubscribeHandler
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode299
app/tweetsnip.go:299: func HandleTweet(ctx appengine.Context, m *mail.Message)
error {
On 2014年05月05日 00:49:00, adg wrote:
> tweetHandler
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode314
app/tweetsnip.go:314: return fmt.Errorf("save tweet: %v", err)
On 2014年05月05日 00:49:02, adg wrote:
> delete the if and just 'return err'
> you should make the error message pretty outside the transaction, if at all
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode327
app/tweetsnip.go:327: http.Error(w, "oops", http.StatusInternalServerError)
On 2014年05月05日 00:49:01, adg wrote:
> oops?
> err.Error() please
Done.
https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode329
app/tweetsnip.go:329: return
On 2014年05月05日 00:49:02, adg wrote:
> delete
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go
File mailmux/mailmux.go (right):
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode18
mailmux/mailmux.go:18: // ServeHTTP parses the incoming message and calls the
handler logging any errors.
On 2014年05月05日 00:49:02, adg wrote:
> s/errors/error/
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode22
mailmux/mailmux.go:22: defer r.Body.Close()
On 2014年05月05日 00:49:02, adg wrote:
> put this above the ReadMessage line
> or drop the defer
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode28
mailmux/mailmux.go:28: ctx.Errorf("mail to %v: %v", r.URL.Path, err)
On 2014年05月05日 00:49:02, adg wrote:
> the path will be in the logs, probably not worth including here
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode34
mailmux/mailmux.go:34: type Router struct {
On 2014年05月05日 00:49:02, adg wrote:
> call this Mux
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode40
mailmux/mailmux.go:40: // Creates a new Router given the prefix in all mail
request paths.
On 2014年05月05日 00:49:02, adg wrote:
> New creates a returns a new Mux that handles email on the given path.
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode41
mailmux/mailmux.go:41: func NewRouter(prefix string) *Router {
On 2014年05月05日 00:49:02, adg wrote:
> s/prefix/path/
> 
> But should you just hardcode this as "/_ah/mail/"?
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode41
mailmux/mailmux.go:41: func NewRouter(prefix string) *Router {
On 2014年05月05日 00:49:02, adg wrote:
> call this New (mailmux.New)
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode53
mailmux/mailmux.go:53: // Router is an http.Handler.
On 2014年05月05日 00:49:02, adg wrote:
> // ServeHTTP implements the http.Handler interface
Done.
https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode55
mailmux/mailmux.go:55: mail := strings.TrimPrefix(r.URL.Path, m.prefix)
On 2014年05月05日 00:49:02, adg wrote:
> s/mail/addr/
Done.