Hongbin - I've just realized that this review request wasn't targeted to a specific group, and "people" was set to just me.

Could you add the "reviewboard" reviewer group to this review request?

Thanks,

-Mike

test

Hongbin:

Sorry for the delay on this.  Thanks for posting it!  I'm really impressed with this, considering the computer issues you've been having, and Python being a new language to you!

I have a few comments/questions below.  I got the extension posting comments on a GoogleCode Project I set up (http://code.google.com/p/rbbugtracker/issues/detail?id=1) which was great!  I had to monkey around a little though, since my RB's project name is "Review Board", and the GoogleCode Project name was rbbugtracker.  I mention that issue again below.

I reviewed about 3/4 of this, and I hope to do the last quarter tomorrow sometime.

Thanks,

-Mike

Seeing this, it makes me wonder:

What if we have GoogleCodeBugtracker, BugzillaBugtracker, etc, all separate extensions - but all of them *require* RBBugTracker?

This way, if I just want to use GoogleCodeBugtracker, I install it, it automatically installs RBBugTracker and imports the libraries from it, etc.

That'd mean you could skip having RBBugTrackerExtension *knowing* about all of those various bug-tracker extensions.

Thoughts?  It might not be practical to try something like that before the end of semester, but it might be the way to take this thing.

Correct me if I'm wrong, but since extension was declared outside of the scope of RBBugTracker, I don't think you need to declare it global.

Same as above.

Instead of printing this to console, we should be logging problems like this, and returning a useful error message.

same as above.

You'll want to remove this before committing.

As stated elsewhere - we might want to make this customizable.

You'll want to remove this before committing.

You'll want to remove this before committing.

Ok, I think I see what you're doing.

In this case, I think you could simplify with:

if BUG_TRACKER_INFO[name]['host'] in review_request.repository.bug_tracker:

If something goes wrong, it needs to be logged.

Anyone feel free to correct me on this - but for stuff like this, we'll probably want Review Board to carry on, even if it failed here.  It should log the info...but it shouldn't break if we couldn't contact GoogleCode, for instance.

If we want regularity and re-use, we might want GoogleCode to subclass from a generic BugTracker class...

not needed

I think you can get this with extension.settings

This could be a problem.

My project could be called "Mike's Great Project" - but Google Code doesn't let me have a project name with spaces.

We might want to add a step in the configuration that has users specify the name of their project on Google Code.

This is also something that we might want customizable.

You'll want to get rid of these, of course.

Outside classes, functions should be separated by two spaces.

Should be:

from googlecode import thing, thing2, thing3, etc.

You don't need "import googlecode"

Please alphabetize your imports - that goes for each of these .py files.

can probably be on the same line, if under 80 chars

Again - see my comment about URL dispatching, and maybe glance elsewhere around Review Board to see how URLs are done.

We might want to extract that magic string and put it up as a constant after the imports.  That's what I tend to do, anyhow.

Hrm.  True/False boolean values are probably simpler, then you could do:

review_checked = get_review_checked(), etc...

you don't need this line.

these can probably be on the same line - so long as it's under 80 char.

space before the for block

I think there might be an easier way to do this.  I'm not 100% on url dispatching in Django though...

maybe give this a quick glance: http://docs.djangoproject.com/en/dev/topics/http/urls/

get_absolute_url, reverse, and resolve strike me as possibly important keywords.

whitespace

I didn't declare it global before, but I found it is wrong. Without the global, python will think "extension" is an local variable.

Hongbin:

I'm not experiencing the same difficulty with reverse that you are.  See below.  Can you send me some URL's with documentation on the error you've mentioned?

Thanks,

-Mike

You've imported "include" without using it, and it can be removed.

Also, two spaces between "from django.conf..." and "urlpatterns =" please.

Strange - I'm not getting those failures that you're mentioning.  100% of the time, reverse('RBBugTracker.googlecode.views.save_username') generated '/RBBugTracker/googlecode/save_username/' for me.

Also, if you're referring to the include in googlecode/urls.py - you're not actually using it.  You've imported it, but it's unused.  I've tagged it with it's own comment.

Sorry. I am wrong. The 20% failed was not happened here. I am OK to change to reverse() here. The 20% failed is on another view.py. The url should be "http://localhost:8080/admin/extensions/RBBugTracker.extension.RBBugTracker/config/"

Here is the problem. If I replace with reverse() here. I will get an about 20% exception.