Make some improvements to our changesets code.

Review Request #4236 — Created June 12, 2013 and submitted

Information

Review Board
master

Reviewers

Make some improvements to our changesets code.

The existing stuff for changesets is pretty heavily biased towards
Perforce--from assuming that the changeset ID will only be a number, to doing
some queries that will only make sense for servers that support pending
changesets with a server-side representation.

This change adds a "supports_pending_changesets" property to scmtools and gates
things based on that.
Ran unit tests, used this in conjunction with my post-commit branch.
Description From Last Updated

I think this could look a little more cleaner and easier to follow as: if not scmtool.supports_pending_changesets: raise NotImplementedError changeset …

chipx86chipx86

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/scmtools/perforce.py
        reviewboard/scmtools/models.py
        reviewboard/scmtools/core.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Show all issues
    Col: 5
     E303 too many blank lines (2)
    
  3. 
      
chipx86
  1. Looks good. Just one thing I think would help code flow.
  2. reviewboard/reviews/models.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
     
    Show all issues
    I think this could look a little more cleaner and easier to follow as:
    
    
    if not scmtool.supports_pending_changesets:
        raise NotImplementedError
    
    changeset = ...
    
    if not changeset or not changeset.pending:
        raise InvalidChangeNumberError
    
    self.update_from_pending_change(...)
    1. This is going to change with the post-commit stuff, so I'd rather not at this point.
    2. Fair enough. Ship it.
  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (4790830).