Intermedia review for GoogleCode issue tracker integration

Review Request #1914 — Created Nov. 14, 2010 and discarded

hongbin
rb-extension-pack
193
reviewboard
mike_conley
The project is still under development. This is an intermediate for some free-back.


HO
  1. 
      
  2. 
      
HO
HO
mike_conley
  1. 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
  2. 
      
HO
GU
  1. test
    1. Please use http://demo.reviewboard.org/ for tests. Thanks!
  2. 
      
mike_conley
  1. 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
  2. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    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.
    1. I think this way will be much cleaner. Actually, if you look back to my original proposal, you will find that I proposed to implement one extension for each bug tracker. However, the feedback I got is to implement one extension, which knows all the bug trackers. Therefore, the RBBugTracker have to know all bug trackers in advance. 
    2. Ah - we must have miscommunicated.  I remember proposing the multi-extension approach during the code sprint, when we were working at the chalkboard.
      
      Regardless, I think it's too late to make any sort of course correction on this, at least with respect to your UCOSP project.  So let's not worry about this.
  3. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    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.
    1. I didn't declare it global before, but I found it is wrong. Without the global, python will think "extension" is an local variable.
    2. You're absolutely right - I was mistaken.  Disregard.
  4. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    Same as above.
  5. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
     
    Instead of printing this to console, we should be logging problems like this, and returning a useful error message.
  6. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    same as above.
  7. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    You'll want to remove this before committing.
  8. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    As stated elsewhere - we might want to make this customizable.
  9. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    You'll want to remove this before committing.
  10. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    You'll want to remove this before committing.
  11. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
     
    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:
  12. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    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.
    1. Accepted. I will log it.
  13. If we want regularity and re-use, we might want GoogleCode to subclass from a generic BugTracker class...
    1. That's the big discussion for the composition VS inheritance. I preferred composition because any change in base class won't affect derived class. In addition, I don't see any method we need to to reuse. What we need in the concrete derived class is a handler method we can call.
    2. I mean the purpose for creating an base class is that we want to reuse method in base class. However, I don't need any method to be reused in this case, so I prefer composition in this case.
    3. That's fine - we'll keep this approach for now, though it might get changed after your project is complete.
    4. I can try the following if you are OK with that. Define a stub base class, with only one function, _notify_bug_tracker(). It raise notimplement exception in the base class. Every bug trackers subclass must implement this function. It just like interface in java. How about that.
    5. Hongbin:
      
      That sounds fine to me - thanks!
      
      -Mike
  14. not needed
    1. Maybe, I will verify that.
  15. I think you can get this with extension.settings
  16. 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.
    1. I see. Accepted. I will try to do it as you suggested.
    2. Agree. But extra step is not necessary because project name is specified at Admin->Database->Repository->BugTracker->Project's name. I will try to get the name from there.
  17. This is also something that we might want customizable.
  18. RBBugTracker/RBBugTracker/googlecode/googlecode.py (Diff revision 3)
     
     
     
     
     
     
    You'll want to get rid of these, of course.
    1. For sure. Accepted
  19. Outside classes, functions should be separated by two spaces.
  20. Should be:
    
    from googlecode import thing, thing2, thing3, etc.
    
    You don't need "import googlecode"
  21. Please alphabetize your imports - that goes for each of these .py files.
  22. can probably be on the same line, if under 80 chars
  23. Again - see my comment about URL dispatching, and maybe glance elsewhere around Review Board to see how URLs are done.
    1. You mean using Reverse or premalink? I tried that, but it didn't work. The reason is I used include in this module so reverse can't be used.
    2. Hm.  Not 100% sure what you mean by "include"...
      
      So I was able to use:
      
      reverse('RBBugTracker.googlecode.views.save_username')
      
      which generated:
      
      '/RBBugTracker/googlecode/save_username/'
      
      And I think I'd go that route.  I'd rather have Django construct URLs, than do it manually.
      
      Also - I think "next" is a built-in Python keyword.  Probably shouldn't override.
    3. I agree the "next" thing. 
      
      For the reverse issue, at the beginning, I used reverse() to get the url. But it gave me about 25% failed. It is a very strange error. I googled that, I found there are several error reports for the combination of reverse() and include(). Therefore, I try to use current approach instead of reverse(). It yields 100%.
    4. By replacing with reverse() here. I still got the 20% failed. I attached the screen-shot. Therefore, I won't use reverse here to make sure it worked. 
  24. 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.
  25. RBBugTracker/RBBugTracker/googlecode/views.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
    Hrm.  True/False boolean values are probably simpler, then you could do:
    
    review_checked = get_review_checked(), etc...
    1. That's is the code to put in HTML. What I mean is I need something like "<input type="checkbox" name=.... checked="yes"/>"
    2. Ah, I see.
      
      I think we'd probably want to use booleans instead, and do something more like this in the form:
      
      <input id="id_shipit" class="recommendation" type="checkbox" {% if review.ship_it %}checked="checked"{% endif %} />
      ^-- from reviewboard/templates/reviews/review_draft_inline_form.html
    3. Accepted
  26. RBBugTracker/RBBugTracker/views.py (Diff revision 3)
     
     
    you don't need this line.
  27. RBBugTracker/RBBugTracker/views.py (Diff revision 3)
     
     
     
    these can probably be on the same line - so long as it's under 80 char.
  28. RBBugTracker/RBBugTracker/views.py (Diff revision 3)
     
     
    space before the for block
  29. RBBugTracker/RBBugTracker/views.py (Diff revision 3)
     
     
     
     
    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.
    1. I answered that somewhere. Because I used include, reverse won't work.
  30. RBBugTracker/setup.py (Diff revision 3)
     
     
    whitespace
  31. 
      
HO
  1. 
      
  2. RBBugTracker/RBBugTracker/extension.py (Diff revision 3)
     
     
    I didn't declare it global before, but I found it is wrong. Without the global, python will think "extension" is an local variable.
  3. 
      
mike_conley
  1. 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
  2. You've imported "include" without using it, and it can be removed.
    
    Also, two spaces between "from django.conf..." and "urlpatterns =" please.
  3. 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.
    
  4. 
      
HO
  1. 
      
  2. 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/"
  3. RBBugTracker/RBBugTracker/views.py (Diff revision 3)
     
     
     
     
    Here is the problem. If I replace with reverse() here. I will get an about 20% exception.
    1. Ok, for this case, leave it as is.  This bit will probably change when we do some architectural overhaul later on.  Thanks, Hongbin.
  4. 
      
HO
HO
HO
Review request changed

Status: Discarded

Loading...