Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(108)

Issue 738: Started adapting old Browser code to work with GAE backend and XmlHttpRequests instead of JSONP

Can't Edit
Can't Publish+Mail
Start Review
Created:
18 years, 1 month ago by rob.ewaschuk
Modified:
3 years, 5 months ago
Reviewers:
fergald
Base URL:
http://wto-informals.googlecode.com/svn/trunk/wto-informals/
Visibility:
Public.

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+103 lines, -35 lines) Patch
app.yaml View 1 chunk +1 line, -1 line 0 comments Download
static/browser.css View 2 chunks +2 lines, -1 line 0 comments Download
static/browser.js View 8 chunks +38 lines, -27 lines 0 comments Download
templates/browser.html View 3 chunks +4 lines, -1 line 0 comments Download
wto-informals.py View 4 chunks +58 lines, -5 lines 4 comments Download

Messages

Total messages: 4
rob.ewaschuk
hey. uhh, this will be mostly greek to you, but take a look if you ...
18 years, 1 month ago (2008-05-07 23:27:18 UTC) #1
rob.ewaschuk
18 years, 1 month ago (2008-05-09 19:31:07 UTC) #2
rob.ewaschuk
18 years, 1 month ago (2008-05-09 19:33:33 UTC) #3
fergald
18 years, 1 month ago (2008-05-09 20:59:04 UTC) #4
not a whole lot to comment on

http://codereview.appspot.com/738/diff/1/5
File wto-informals.py (right):

http://codereview.appspot.com/738/diff/1/5#newcode18
Line 18: 
my first reaction was to say "make this an object". I still think it's right but
it doesn't buy you a whole lot.

I'm not sure about the temp_foo stuff. are you trying to make rebuilding the
cache atomic? If so, you still have a problem with having to set 2 variables at
the end.

Making it an object with all this going on inside it's __init__ would solve this
- you'd just say

members_cache = MembersCache()

and rebuilding it would just be reassigning a new instance to the global - which
would be atomic.

http://codereview.appspot.com/738/diff/1/5#newcode27
Line 27: memberships = Membership.all()
redundant variable

http://codereview.appspot.com/738/diff/1/5#newcode103
Line 103: def get(self, group):
what value does group get?

http://codereview.appspot.com/738/diff/1/5#newcode183
Line 183: #if not curUser:
I don't see any other user checking in the code. I've started playing with
appengine and almost the first thing I did was

class UserRequestHandler(webapp.RequestHandler):
  def get(self):
    user = users.get_current_user()
    if not user:	
      self.redirect(users.create_login_url(self.request.uri))
    else:
      self.request.user = user
      self.user_get()

or some-such
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b