Custom post-review implementations making them easier proposal

[Chris Clark]

If you want to deploy post-review with some site specific customizations
(e.g. add support for a custom in house SCM) there are only a few options:

* Privately fork post-review and try and sync all the changes that
occur in "trunk" into your version
* add a wrapper script around post-review, e.g. rename post-review
to postreview.py (python does not allow hypens "-" in
module/library names), import postreview in your tool and then
monkey patch in your changes. The rename can be handled in
setup.py when "building"

Neither solution is optimal but this is the price you pay if you want
private custom versions :-(

I don't have a solution to the above but I have some ideas on making it
suck slightly less by making either option easier to deal with. For
instance take the case where one needs to add in support for a custom
SCM. You need to add a new class (subclassed from SCMClient) and then
modify determine_client() to add it to the list of SCM's.

* With the forked approach you have to edit determine_client().
* With the wrapper approach you have to copy the existing
determine_client() into your wrapper, modify it then monkey patch
the original.

Either way you have to be prepared to follow the trunk version in case
determine_client() changes.

What do people think of the following ideas:

1) move the tuple of SCM clients into a global variable.
E.g. instead of:

# Try to find the SCM Client we're going to be working with.
for tool in (SVNClient(), CVSClient(), GitClient(), MercurialClient(),
PerforceClient(), ClearCaseClient()):

Have:

scm_tuple=(SVNClient(), CVSClient(), GitClient(), MercurialClient(),
PerforceClient(), ClearCaseClient())
....
def determine_client():
....
# Try to find the SCM Client we're going to be working with.
for tool in scm_tuple:

This makes monkey patching the list easier, you just update the global
from outside.

2) modify the trunk version of post-review to change the formatting that
is used in the tuple (list) of SCM clients to make it easier to edit the
list. E.g.:

scm_tuple=(
SVNClient(),
CVSClient(),
GitClient(),
MercurialClient(),
PerforceClient(),
ClearCaseClient(),
)

This makes it easier to modify the code if you decide to fork the
script. You can re-order, add, or remove very easily. Diffs show up very
clearly.

Does this appeal to anyone? #1 above would make my life easier. I wanted
to get a feel from a wide audience before posting a for patch for review
where it may not be seen by so many people.

Chris

[Ritesh Nadhani]

For one, I would be interested. We use Bazaar and and have been slowly
trying to intergrate ReviewBoard in our system. I looked into the
post-review and it does not seem to support Bazaar. I have already
extended it using the Mercurial example and override two or three
methods that the Mercurial component did.

I plan to soon post my changes for Bazaar once I can verify that my
code actually works :)

--
Ritesh
http://www.riteshn.com

[Chris Clark]

Ritesh Nadhani wrote:
> For one, I would be interested. We use Bazaar and and have been slowly
> trying to intergrate ReviewBoard in our system. I looked into the
> post-review and it does not seem to support Bazaar. I have already
> extended it using the Mercurial example and override two or three
> methods that the Mercurial component did.
>
> I plan to soon post my changes for Bazaar once I can verify that my
> code actually works :)
>

Thanks for the feedback. I've not heard any "nays" so I'm guessing no
one hates the idea :-)
I went ahead and created:

http://reviews.review-board.org/r/882/

Chris

[Christian Hammond]

Yep, sounds good to me.

I'm out of town and am not as available as I normally am, so it might still be a few days before I start reviewing code, but the proposal is good.

I have some further thoughts for making post-review more extensible, but I'd need the time to do it properly. What I'd like to do is create an rbtools namespace that post-review and other future scripts would use. It would contain the SCMClients and would be done in such a way where we could plug into the extensibility support Python setuptools gives us, where other packages could register "entry points" for things. We'd support our built-in SCMClients and then load any provided by an entry point, meaning that adding third-party support for some repository is as simple as writing an installable Python module. Of course, we'd still want people to contribute to RBTools directly.

I also would like an rbapi (or something) package for all the server communication code. This would be separate from RBTools, but RBTools would use it. post-review would be greatly simplified by these two changes.

I'd like to take all this a step further and provide a good way of specifying defaults for post-review without modifying anything. There are a few options for this:

1) User/system configuration files containing defaults. We'd need a good way of determining the path for the system config file, as it may vary (may want to keep it on network storage, for instance). Could even check the directory post-review is in.

2) Look for an optional rbtools_config.py in the PYTHON_PATH. This could be installed on the local user's system, or on a network share somewhere. RBTools wouldn't care, so long as PYTHON_PATH is set to wherever it is.

3) Both. Why not.

Christian

--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.review-board.org
VMware, Inc. - http://www.vmware.com

[Chris Clark]

Christian Hammond wrote:
> ...I also would like an rbapi (or something) package for all the

> server communication code. This would be separate from RBTools, but
> RBTools would use it. post-review would be greatly simplified by these
> two changes.

That would be awesome, we've had a couple of ideas for enhancements to
post-review on modifying existing fields for a review (i.e. lookup
followed by change) but the guys who want to work on it, are not web
service people.

> I'd like to take all this a step further and provide a good way of
> specifying defaults for post-review without modifying anything. There
> are a few options for this:
>
> 1) User/system configuration files containing defaults. We'd need a
> good way of determining the path for the system config file, as it may
> vary (may want to keep it on network storage, for instance). Could
> even check the directory post-review is in.
>
> 2) Look for an optional rbtools_config.py in the PYTHON_PATH. This
> could be installed on the local user's system, or on a network share
> somewhere. RBTools wouldn't care, so long as PYTHON_PATH is set to
> wherever it is.
>
> 3) Both. Why not.

How about:

1. Check for some environment variable that points to a config file,
e.g. POST_REVIEW_CONF=/fullpath/filename.something
2. no environment variable? Default to a search path for a
specifically named file; home directory, current directory, path
for post-review, python path (maybe), etc.

The names / order above would need to be thrashed out, it is more the
idea that you can override with a search initiated if there is no setting.

Chris