-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Why return None, when data.strip() [or a local variable holding its value] already has a perfectly valid Mercurial repository path?
Edited the MercurialBackend to support the repo defined in .hg/hgrc
Review Request #255 — Created Feb. 15, 2008 and submitted
Information | |
---|---|
apollo13 | |
Review Board SVN (deprecated) | |
Reviewers | |
reviewboard | |
Edited the MercurialBackend to support the repo defined in .hg/hgrc
LE
LE
-
[I do not know if it is OK for third-party people (that is, those who are not core developers) to do reviews here. If it is considered impolite or impractical, please say so and forgive me.] There are small problems on the updated patch, as commented below. But I would like to insist that a command line switch to let the user specify if he wants to use the path of the local or remote depository would be better, because random changes on hgrc would not change the behaviour of post-review. And people using a default "remote" repository could still use the local path (that is not my use case, but it seems possible).
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 2) This should be data.strip(). I would also name the variable 'local_repository' or something similar and use it on the line 800 instead of calling data.strip() again.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 2) I think this comment should be keep just after the end of the previous if.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 2) The second parameter of os.path.join should be split in two: '.hg' and 'hgrc'.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 2) I missed this return on the previous review. If the selected strategy is to fall-back to the previous behaviour, this should not be a 'return None'.