Issue 91000044: tweetsnip
Descriptiontweetsnip
Patch Set 1 #
Total comments: 83
MessagesTotal messages: 3
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 s/showing/that shows/ 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)@.* why not just use "/_ah/mail/.*" and have mailmux send the 404? 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. set s/length // https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode33 app/tweetsnip.go:33: gaemail "appengine/mail" the common renaming is "netmail" for "net/mail" and leaving "appengine/mail" as "mail" 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. Snippet is a bad name for this; it is singular, and is not a collective noun. why not just "Group" https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode60 app/tweetsnip.go:60: func LatestSnippet(ctx appengine.Context) (*Snippet, error) { 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. https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode69 app/tweetsnip.go:69: p := &Snippet{1} add a comment: // Create a snippet if it does not exist. https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode77 app/tweetsnip.go:77: // tweetsInSnippet returns all the tweets in the given snippet. comment doesn't match function name https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode82 app/tweetsnip.go:82: return nil, fmt.Errorf("get unseen snippets: %v", err) what's the meaning of this error message? https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode88 app/tweetsnip.go:88: func (p *Snippet) Put(ctx appengine.Context) error { this method doesn't really add anything https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode99 app/tweetsnip.go:99: Content string // Content of the tweet. "Tweets" are supposed to be short, but this will be hard limited to 255 unless you add a struct tag: `datastore:",noindex"` 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. "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. https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode123 app/tweetsnip.go:123: // subscriptions returns the list of active subscriptions. 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; https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode125 app/tweetsnip.go:125: var usrs []User s/usrs/users/ https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode131 app/tweetsnip.go:131: to := make([]string, 0, len(usrs)) don't preallocate the array. this happens twice a week var to []string https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode132 app/tweetsnip.go:132: for _, usr := range usrs { s/usr/u/ https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode133 app/tweetsnip.go:133: to = append(to, usr.Email) if you _do_ preallocate an array (and you needn't here), you should use for i, u := range users { to[i] = u.Email } https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode157 app/tweetsnip.go:157: ctx.Infof("no active users, skipping email") s/active // https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode157 app/tweetsnip.go:157: ctx.Infof("no active users, skipping email") s/email/reminder/ https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode171 app/tweetsnip.go:171: goodGuys := make(map[string]bool) goodguys badguys? cute but bad for readability "seen" and "to" https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode188 app/tweetsnip.go:188: "So what's awesome?", make these string constants at the top of the file https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode193 app/tweetsnip.go:193: Last week some cool things happened! "Here's what happened last week:" ? 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 wrap these comments to 80 columns https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode207 app/tweetsnip.go:207: ctx.Infof("no active users, skipping snippets") s/active // 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()) IIRC emails aren't considered part of a transaction, so you should send the mail once the transaction has completed successfully. 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. readBody returns the plain text component of the provided message. https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode276 app/tweetsnip.go:276: s, e := strings.Index(res, "<"), strings.Index(res, ">") there's a function in net/mail to parse these lines https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode284 app/tweetsnip.go:284: func SubscribeUser(ctx appengine.Context, m *mail.Message) error { subscribeHandler or handleSubscribe https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode292 app/tweetsnip.go:292: func UnsubscribeUser(ctx appengine.Context, m *mail.Message) error { unsubscribeHandler https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode299 app/tweetsnip.go:299: func HandleTweet(ctx appengine.Context, m *mail.Message) error { tweetHandler https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode314 app/tweetsnip.go:314: return fmt.Errorf("save tweet: %v", err) delete the if and just 'return err' you should make the error message pretty outside the transaction, if at all https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode327 app/tweetsnip.go:327: http.Error(w, "oops", http.StatusInternalServerError) oops? err.Error() please https://codereview.appspot.com/91000044/diff/1/app/tweetsnip.go#newcode329 app/tweetsnip.go:329: return delete 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. s/errors/error/ https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode22 mailmux/mailmux.go:22: defer r.Body.Close() put this above the ReadMessage line or drop the defer 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) the path will be in the logs, probably not worth including here https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode34 mailmux/mailmux.go:34: type Router struct { call this Mux 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. New creates a returns a new Mux that handles email on the given path. https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode41 mailmux/mailmux.go:41: func NewRouter(prefix string) *Router { call this New (mailmux.New) https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode41 mailmux/mailmux.go:41: func NewRouter(prefix string) *Router { s/prefix/path/ But should you just hardcode this as "/_ah/mail/"? https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode53 mailmux/mailmux.go:53: // Router is an http.Handler. // ServeHTTP implements the http.Handler interface https://codereview.appspot.com/91000044/diff/1/mailmux/mailmux.go#newcode55 mailmux/mailmux.go:55: mail := strings.TrimPrefix(r.URL.Path, m.prefix) s/mail/addr/
Sign in to reply to this message.
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.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
