goconf reorganization

goconf reorganization

[adg, 2013-09-03 22:40:48]

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go
File app/conf/consts.go (right):

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go#newcode1
app/conf/consts.go:1: package conf
I think consts is a bad name for this file. they're not constants

data.go ?

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go#newcode9
app/conf/consts.go:9: var (
don't use a factored var block here, it looks weird


var topicList ...

var cityList ...

var conferenceList...

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go
File app/conf/http.go (right):

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go#newcode33
app/conf/http.go:33: http.Handle("/scheduleconference",
handler(scheduleConfHandler))
nicer paths, please!

/conf/schedule
/conf/save
/conf/list
/conf/notify

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go#newcode39
app/conf/http.go:39: http.Handle("/developer", handler(developerHandler))
/admin
adminHandler

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go#newcode42
app/conf/http.go:42: http.Handle("/showtickets", handler(showTicketsHandler))
/ticket
/ticket/buy

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go#newcode46
app/conf/http.go:46: http.Handle("/userprofile",
authHandler(userProfileHandler))
/user
/user/save
/cal

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go
File pkg/conf/conf.go (right):

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode23
pkg/conf/conf.go:23: LatestAnnouncementkey = "LatestAnnouncement" // Memcache
key for the latest announcement
s/key/Key/

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode25
pkg/conf/conf.go:25: AnnouncementKind = "Announcement"   // Datastore kind for
Announcement
these comments are kinda redundant, perhaps just one comment at the top?

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode60
pkg/conf/conf.go:60: MaxAttendees    int64
int64? really?
i somehow doubt any conference will need more than an int's worth (1/3rd the
world population) of tickets

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode61
pkg/conf/conf.go:61: NumTixAvailable int64
TixAvailable

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode82
pkg/conf/conf.go:82: // LoadConference loads a conference from the datastore
given a datastore key.
s/L/l/

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode86
pkg/conf/conf.go:86: return nil, fmt.Errorf("get conference: %v", err)
the message should be "loadConference: %v"

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode86
pkg/conf/conf.go:86: return nil, fmt.Errorf("get conference: %v", err)
formatting the err message makes it impossible to compare against
datastore.ErrNoSuchEntity... seems a bit of a drawback?

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode96
pkg/conf/conf.go:96: if conf.key != nil {
do these the other way around; avoids an allocation if conf.key != nil

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode104
pkg/conf/conf.go:104: // If the Conference was already in the datastore we
finish.
I suspect this is a stale comment

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode145
pkg/conf/conf.go:145: Sender:  "campoy@golang.org",
put this in a named constant

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode147
pkg/conf/conf.go:147: Subject: "Conference you might be interested in: " +
conf.Name,
ditto

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode148
pkg/conf/conf.go:148: Body: fmt.Sprintf("Hi! \n We want to let you know that a
conference called %s "+
use text/template ?

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode162
pkg/conf/conf.go:162: Status   TicketStatus
s/Status/State/g

check out the dictionary definitions of "state" and "status". It's shocking how
often "status" is misused in software.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode169
pkg/conf/conf.go:169: // TicketStatus represents the status of a conference
ticket.
TicketState represents the state of a conference ticket.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode223
pkg/conf/conf.go:223: conf.NumTixAvailable--
this looks wrong
you need to perform both the get and put for the conference entity within the
same transaction for this decrement to work correctly (it's racy otherwise)

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode240
pkg/conf/conf.go:240: func (conf *Conference) LoadAvailableTickets(ctx
appengine.Context) ([]Ticket, error) {
w/Load//

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode281
pkg/conf/conf.go:281: if err := memcache.JSON.Set(ctx, item); err != nil {
It seems a mistake to set the memcache record before the datastore put.
If the put fails, we shouldn't see the new announcement
On the other hand, it's okay to see the old announcement until the cache expires

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode312
pkg/conf/conf.go:312: a.Save(ctx)
huh? why Put the datastore record again? just to the memcache set here

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode323
pkg/conf/conf.go:323: tickets []Ticket `datastore:"-"`
no need for the struct tag; it's an unexported field so datastore can't see it

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go
File pkg/tmpl/tmpl.go (right):

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode8
pkg/tmpl/tmpl.go:8: package tmpl
why is this in a separate package, as opposed to just part of the app?

it's not generally useful because it only has one central template value

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode26
pkg/tmpl/tmpl.go:26: "date":    func(d time.Time) string { return d.Format("2006
Jan 2") },
make this a top-level function

although if it's only used once or twice, you can just write

{{.t Format "2006 Jan 2"}}

in the template

[francesc, 2013-09-05 00:00:25]

PTAL

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go
File app/conf/consts.go (right):

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go#newcode1
app/conf/consts.go:1: package conf
On 2013/09/03 22:40:48, adg wrote:
> I think consts is a bad name for this file. they're not constants
> 
> data.go ?

Done.

https://codereview.appspot.com/12975045/diff/5001/app/conf/consts.go#newcode9
app/conf/consts.go:9: var (
On 2013/09/03 22:40:48, adg wrote:
> don't use a factored var block here, it looks weird
> 
> 
> var topicList ...
> 
> var cityList ...
> 
> var conferenceList...

Done.

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go
File app/conf/http.go (right):

https://codereview.appspot.com/12975045/diff/5001/app/conf/http.go#newcode33
app/conf/http.go:33: http.Handle("/scheduleconference",
handler(scheduleConfHandler))
On 2013/09/03 22:40:48, adg wrote:
> nicer paths, please!
> 
> /conf/schedule
> /conf/save
> /conf/list
> /conf/notify
> 

I kept those paths just because they're the ones they use in the Python and Java
versions ... I agree they're ugly though.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go
File pkg/conf/conf.go (right):

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode23
pkg/conf/conf.go:23: LatestAnnouncementkey = "LatestAnnouncement" // Memcache
key for the latest announcement
On 2013/09/03 22:40:48, adg wrote:
> s/key/Key/

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode25
pkg/conf/conf.go:25: AnnouncementKind = "Announcement"   // Datastore kind for
Announcement
On 2013/09/03 22:40:48, adg wrote:
> these comments are kinda redundant, perhaps just one comment at the top?

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode60
pkg/conf/conf.go:60: MaxAttendees    int64
On 2013/09/03 22:40:48, adg wrote:
> int64? really?
> i somehow doubt any conference will need more than an int's worth (1/3rd the
> world population) of tickets

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode61
pkg/conf/conf.go:61: NumTixAvailable int64
On 2013/09/03 22:40:48, adg wrote:
> TixAvailable

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode82
pkg/conf/conf.go:82: // LoadConference loads a conference from the datastore
given a datastore key.
On 2013/09/03 22:40:48, adg wrote:
> s/L/l/

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode86
pkg/conf/conf.go:86: return nil, fmt.Errorf("get conference: %v", err)
On 2013/09/03 22:40:48, adg wrote:
> the message should be "loadConference: %v"

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode86
pkg/conf/conf.go:86: return nil, fmt.Errorf("get conference: %v", err)
On 2013/09/03 22:40:48, adg wrote:
> formatting the err message makes it impossible to compare against
> datastore.ErrNoSuchEntity... seems a bit of a drawback?

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode96
pkg/conf/conf.go:96: if conf.key != nil {
On 2013/09/03 22:40:48, adg wrote:
> do these the other way around; avoids an allocation if conf.key != nil

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode104
pkg/conf/conf.go:104: // If the Conference was already in the datastore we
finish.
On 2013/09/03 22:40:48, adg wrote:
> I suspect this is a stale comment

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode145
pkg/conf/conf.go:145: Sender:  "campoy@golang.org",
On 2013/09/03 22:40:48, adg wrote:
> put this in a named constant

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode147
pkg/conf/conf.go:147: Subject: "Conference you might be interested in: " +
conf.Name,
On 2013/09/03 22:40:48, adg wrote:
> ditto

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode148
pkg/conf/conf.go:148: Body: fmt.Sprintf("Hi! \n We want to let you know that a
conference called %s "+
On 2013/09/03 22:40:48, adg wrote:
> use text/template ?

Moved to the app code, using text/template.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode162
pkg/conf/conf.go:162: Status   TicketStatus
On 2013/09/03 22:40:48, adg wrote:
> s/Status/State/g
> 
> check out the dictionary definitions of "state" and "status". It's shocking
how
> often "status" is misused in software.

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode169
pkg/conf/conf.go:169: // TicketStatus represents the status of a conference
ticket.
On 2013/09/03 22:40:48, adg wrote:
> TicketState represents the state of a conference ticket.

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode223
pkg/conf/conf.go:223: conf.NumTixAvailable--
On 2013/09/03 22:40:48, adg wrote:
> this looks wrong
> you need to perform both the get and put for the conference entity within the
> same transaction for this decrement to work correctly (it's racy otherwise)

Good catch.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode240
pkg/conf/conf.go:240: func (conf *Conference) LoadAvailableTickets(ctx
appengine.Context) ([]Ticket, error) {
On 2013/09/03 22:40:48, adg wrote:
> w/Load//

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode281
pkg/conf/conf.go:281: if err := memcache.JSON.Set(ctx, item); err != nil {
On 2013/09/03 22:40:48, adg wrote:
> It seems a mistake to set the memcache record before the datastore put.
> If the put fails, we shouldn't see the new announcement
> On the other hand, it's okay to see the old announcement until the cache
expires

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode312
pkg/conf/conf.go:312: a.Save(ctx)
On 2013/09/03 22:40:48, adg wrote:
> huh? why Put the datastore record again? just to the memcache set here
>  

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/conf/conf.go#newcode323
pkg/conf/conf.go:323: tickets []Ticket `datastore:"-"`
On 2013/09/03 22:40:48, adg wrote:
> no need for the struct tag; it's an unexported field so datastore can't see it

Done.

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go
File pkg/tmpl/tmpl.go (right):

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode8
pkg/tmpl/tmpl.go:8: package tmpl
On 2013/09/03 22:40:48, adg wrote:
> why is this in a separate package, as opposed to just part of the app?
> 
> it's not generally useful because it only has one central template value

it's not generally useful, but I think it's useful outside of this application
though.

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode26
pkg/tmpl/tmpl.go:26: "date":    func(d time.Time) string { return d.Format("2006
Jan 2") },
On 2013/09/03 22:40:48, adg wrote:
> make this a top-level function
> 
> although if it's only used once or twice, you can just write
> 
> {{.t Format "2006 Jan 2"}}
> 
> in the template 

Done.

[adg, 2013-09-05 07:17:09]

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go
File pkg/tmpl/tmpl.go (right):

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode8
pkg/tmpl/tmpl.go:8: package tmpl
On 2013/09/05 00:00:26, gocampoy wrote:
> On 2013/09/03 22:40:48, adg wrote:
> > why is this in a separate package, as opposed to just part of the app?
> > 
> > it's not generally useful because it only has one central template value
> 
> it's not generally useful, but I think it's useful outside of this application
> though.

I think it might be generally useful, but practically you're not gaining much
with this package. I really suggest you move this stuff into the main app
package

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode46
pkg/tmpl/tmpl.go:46: return fmt.Errorf("oops: %v", err)
terrible error message

https://codereview.appspot.com/12975045/diff/5001/pkg/tmpl/tmpl.go#newcode56
pkg/tmpl/tmpl.go:56: return fmt.Errorf("no!: %v", err)
worse

https://codereview.appspot.com/12975045/diff/16001/README.md
File README.md (right):

https://codereview.appspot.com/12975045/diff/16001/README.md#newcode4
README.md:4: Conference manager on the cloud using Go and Google Cloud
flesh this out please

https://codereview.appspot.com/12975045/diff/16001/app/conf/page.go
File app/conf/page.go (right):

https://codereview.appspot.com/12975045/diff/16001/app/conf/page.go#newcode13
app/conf/page.go:13: type Page struct {
comments

https://codereview.appspot.com/12975045/diff/16001/app/templates/email.tmpl
File app/templates/email.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/email.tmpl#n...
app/templates/email.tmpl:3: We want to let you know that a conference called
{{.Name}} has been scheduled to start on {{.Format "2006-01-02"}} in {{.City}}.
you should wrap lines at 70 columns in emails

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
File app/templates/show_calendar.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
app/templates/show_calendar.tmpl:9: <h1>Your calendar events</h1>
perhaps just "Your calendar"

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
app/templates/show_calendar.tmpl:12: <p>Logged in user is {{.User.Email}} </p>
inconsistent indentation
i suggest you use tabs throughout

https://codereview.appspot.com/12975045/diff/16001/app/templates/tickets.tmpl
File app/templates/tickets.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/tickets.tmpl...
app/templates/tickets.tmpl:14: <p>Ticket number {{.Number}} is {{ .State }} <a
href="/buyticket?ticket_key_str={{ .ID }}"> [Purchase] </a></p>
indent

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go
File pkg/auth/auth.go (right):

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode21
pkg/auth/auth.go:21: const CallbackURL = "/oauth2callback"
this is a path, not a url

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode35
pkg/auth/auth.go:35: func Handle(url string, h http.Handler, cfg *oauth.Config)
error {
is this a url or a path?

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode83
pkg/auth/auth.go:83: status := r.FormValue("status")
what's "status"? some oauth thing? I thought it was "state"?

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go
File pkg/conf/conf.go (right):

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go#newcode5
pkg/conf/conf.go:5: // The package conf provides functionality for the
conference management
package comments should begin "Package conf..."

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go#newcode5
pkg/conf/conf.go:5: // The package conf provides functionality for the
conference management
"functionality" is vague. isn't it the data models?

[francesc, 2013-09-13 01:14:42]

PTAL

https://codereview.appspot.com/12975045/diff/16001/README.md
File README.md (right):

https://codereview.appspot.com/12975045/diff/16001/README.md#newcode4
README.md:4: Conference manager on the cloud using Go and Google Cloud
On 2013/09/05 07:17:10, adg wrote:
> flesh this out please

I will add more information about the context once Gary has reviewed and given
his approval.

https://codereview.appspot.com/12975045/diff/16001/app/conf/page.go
File app/conf/page.go (right):

https://codereview.appspot.com/12975045/diff/16001/app/conf/page.go#newcode13
app/conf/page.go:13: type Page struct {
On 2013/09/05 07:17:10, adg wrote:
> comments

Done.

https://codereview.appspot.com/12975045/diff/16001/app/templates/email.tmpl
File app/templates/email.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/email.tmpl#n...
app/templates/email.tmpl:3: We want to let you know that a conference called
{{.Name}} has been scheduled to start on {{.Format "2006-01-02"}} in {{.City}}.
On 2013/09/05 07:17:10, adg wrote:
> you should wrap lines at 70 columns in emails

Done.

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
File app/templates/show_calendar.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
app/templates/show_calendar.tmpl:9: <h1>Your calendar events</h1>
On 2013/09/05 07:17:10, adg wrote:
> perhaps just "Your calendar"

Done.

https://codereview.appspot.com/12975045/diff/16001/app/templates/show_calenda...
app/templates/show_calendar.tmpl:12: <p>Logged in user is {{.User.Email}} </p>
On 2013/09/05 07:17:10, adg wrote:
> inconsistent indentation
> i suggest you use tabs throughout

Done with spaces (I reserve tabs for Go)

https://codereview.appspot.com/12975045/diff/16001/app/templates/tickets.tmpl
File app/templates/tickets.tmpl (right):

https://codereview.appspot.com/12975045/diff/16001/app/templates/tickets.tmpl...
app/templates/tickets.tmpl:14: <p>Ticket number {{.Number}} is {{ .State }} <a
href="/buyticket?ticket_key_str={{ .ID }}"> [Purchase] </a></p>
On 2013/09/05 07:17:10, adg wrote:
> indent

Done.

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go
File pkg/auth/auth.go (right):

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode21
pkg/auth/auth.go:21: const CallbackURL = "/oauth2callback"
On 2013/09/05 07:17:10, adg wrote:
> this is a path, not a url

Done.

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode35
pkg/auth/auth.go:35: func Handle(url string, h http.Handler, cfg *oauth.Config)
error {
On 2013/09/05 07:17:10, adg wrote:
> is this a url or a path?

Done.

https://codereview.appspot.com/12975045/diff/16001/pkg/auth/auth.go#newcode83
pkg/auth/auth.go:83: status := r.FormValue("status")
On 2013/09/05 07:17:10, adg wrote:
> what's "status"? some oauth thing? I thought it was "state"?

Done, I replaced state with status by mistake.

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go
File pkg/conf/conf.go (right):

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go#newcode5
pkg/conf/conf.go:5: // The package conf provides functionality for the
conference management
On 2013/09/05 07:17:10, adg wrote:
> "functionality" is vague. isn't it the data models?

Done.

https://codereview.appspot.com/12975045/diff/16001/pkg/conf/conf.go#newcode5
pkg/conf/conf.go:5: // The package conf provides functionality for the
conference management
On 2013/09/05 07:17:10, adg wrote:
> package comments should begin "Package conf..."

Done.

[adg, 2013-09-23 08:05:52]

LGTM