Summary

Make some improvements to our changesets code.

Review Request #4236 — Created June 12, 2013 and submitted June 12, 2013, 10:57 p.m.

Information

Owner

david

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done

Ran unit tests, used this in conjunction with my post-commit branch.

Issues

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 …	chipx86	June 12, 2013, 10:51 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	June 12, 2013, 10:39 p.m.

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:

reviewboard/reviews/models.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Col: 5
 E303 too many blank lines (2)
```

Looks good. Just one thing I think would help code flow.

reviewboard/reviews/models.py (Diff revision 1)

The issue has been dropped. 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(...)

david June 12, 2013, 10:51 p.m.

This is going to change with the post-commit stuff, so I'd rather not at this point.

chipx86 June 12, 2013, 10:53 p.m.
```
Fair enough. Ship it.
```

Status:: Completed
Change Summary:: Pushed to master (4790830).