Re: Custom post-review implementations making them easier proposal
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 - chip...@chipx86.com Review Board - http://www.review-board.org VMware, Inc. - http://www.vmware.com On Fri, Jun 5, 2009 at 1:41 PM, Chris Clark chris.cl...@ingres.com wrote: 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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Custom post-review implementations making them easier proposal
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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Custom post-review implementations making them easier proposal
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 :) On Tue, Jun 2, 2009 at 1:56 PM, Chris Clark chris.cl...@ingres.com wrote: 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 http://www.riteshn.com --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Re: Custom post-review implementations making them easier proposal
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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
Custom post-review implementations making them easier proposal
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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups reviewboard group. To post to this group, send email to reviewboard@googlegroups.com To unsubscribe from this group, send email to reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
