-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Should be sorted alphabetically in the include list.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) This probably doesn't belong in this change.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Two blank lines before this. Also, This should be spelled out as ClearCaseClient.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) How do clearcase views work? Is this an okay assumption to make? What if the Review Board server is talking to multiple clearcase servers?
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Comments should be in sentence form (start with a capital letter, end with a period).
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) This shouldn't be part of this change.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) This should all be removed as well. In fact, this whole function should go away so the default SCMClient implementation can be used to determine the server based on config files.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) If you didn't need the debug output, this could be simplified as: md5.md5(fname).hexdigest()
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Looks like we're just stripping leading "\"s, so we can instead do: s = s.lstrip("\\")
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Shame the unc path code doesn't exist on non-Windows installs of Python... It would be nice to add comments explaining what's going on here a bit. I have to wonder, though.. Since we're already converting "/" to "\", why not make sure we're using "/" first and then do: s = os.path.normpath(s) s = s.replace("/", "\\")
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Does this really make sense? Wouldn't we end up with something like C:\\foo\\bar\\abc?
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Make sure this wraps to about ~75 characters.
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) For things like this, os.path.join is the correct method.
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Do we really need a regex for this? We can't just do: if "@@" in whatever: ?
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Too many spaces after the "=". Also, there's no reason to special-case these on platforms. The code's the same.
-
-
-
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) These are exactly the same. No need to special case on platform.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) This is better done as: return [line.strip() for line in ldesc if line.startswith("/vbos")]
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Only one blank line. Same with other things below. I recommend going through and making sure you're not adding condensed lines.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Better as: return [line.rstrip("\r\n") + "\n" for line in data] How necessary is this, though? We should handle line endings in Review Board natively. I'd be a little concerned with doing this in this code.
-
/trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1) I'm really worried about this causing regressions. Especially when it's perfectly valid for a file to have multiple newlines at the end, and this is assuming we'll only have one. Not to mention that I'm not even sure we'd have any \r\n anyway, given the previous lines. What's the rationale for this?
-
/trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1) We can't have any SCMTool-specific code in this file. This logic will have to be done in the SCMTool somehow.
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) I don't think you really mean to import this.
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) This shouldn't be in this change. If you absolutely need debugging, use the standard Python logging support, with a log level of DEBUG.
-
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) What's the purpose of this? I noticed you're trying to use it in common code, but it should only ever be used internally. I have a hunch you really want to use os.path stuff for this.
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) What's the purpose of the str assignment? And some documentation for the function would be nice, because the name is ambiguous (and doesn't follow the same naming style as other functions).
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) Can you provide docs for this function as well?
-
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) Can collapse these: if 'index' in info and self.lines[linenum] == self.BINARY_STRING:
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) You can pass in filename directly. No need for "%s" % filename.
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) Simpler as: if not failure: return contents if errmsg.startswith(...) ... else: ...
Clearcase module with post-review support
Review Request #653 — Created Dec. 1, 2008 and submitted
Another clearcase integration module with post-review support. It supports windows, linux and cygwin environments, and both snapshot and dynamic views. The module depends on a mounted view on the reviewboard server. It is capable of submit reviews for checked out and checked in elements.
Submitted reviews on linux, windows and cygwin using snapshot and dynamic views.
Screenshots
D.
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) This part of the function basically removes the "previous dir (..)" and "this dir (.)" redirections. For example: /root/dir1/dir2/../dir3 becomes /root/dir1/dir3
D.
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) Yes, we will. But this is necessary because we use this string later on, the single \ just disappear.
D.
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 1) I got lots of problems with line endings. I tried to create a common way to create the diff so it won't show the whole file in the post review server or fail to apply the patch. It just doesn't work well if I don't handle this problem.
D.
-
-
/trunk/reviewboard/diffviewer/diffutils.py (Diff revision 1) Any idea? I just need to remove all the extended path data to display something readable.
D.
Change Summary:
I made most of the requested changes. I tried to eliminate as much as possible the platform-dependent choices. I fixed the blank lines, comments and line breaks. There is a couple of debug statements that will be removed on the next review. Most of the changes were on the post-review tool. I couldn't address the diffutils problem yet. I may need some suggestions in this case.
Diff: |
Revision 2 (+492 -3) |
---|
D.
Change Summary:
1 - Simplified the code. 2 - Label submission will search the vobs for the files; 3 - Fixed some path normalization issues; 4 - Removed regexes on the scmtools/clearcase.py;
Diff: |
Revision 3 (+500 -3) |
---|
-
These comments are mostly stylistic issues:
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Can you align the env= with the ["clear... ?
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Instead of defining so many variables here, just do: curr_version, pre_version = execute(...).split('\n')
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Similarly here, pre_version = pre_version.split(':')[1].strip()
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) If you're going to comment this as 'fname', please call the variable 'fname' as well.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) A few style issues here. I think this would be much clearer written as: bpath = vkey[:vkey.rfind('vobs') + 4] fpath = vkey[vkey.rfind('vobs') + 5:]
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Please indent the vkey line only 4 spaces
-
-
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) No parentheses here: return set(filelist)
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) No parentheses around the return value
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Just write this as one line: temp.append(line.rstrip('\r\n') + '\n')
-
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Formatting is a bit odd here. Can you write it like this? if (self.viewtype == 'snapshot' and (sys.platform.startswith('win') or sys.platform.startswith('cygwin'))): ...
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 3) Is this line longer than 80 characters now?
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 3) Instead of these, you can just do: fpath = ['vobs']
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 3) What about linux filenames that contain :?
D.
Change Summary:
Implemented David's suggestions/fixes and ported the changed to the latest post-review from the server.
Diff: |
Revision 4 (+549 -3) |
---|
-
Just a few trivial comments now.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 4) You've got some trailing whitespace here.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 4) Whitespace here too. Please go through the diff viewer on RB and check for red.
-
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 4) Indent the parameters 4 spaces from the previous block.
-
/trunk/reviewboard/contrib/tools/post-review (Diff revision 4) Indent these 4 spaces past the last block indent.
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 4) Is this supposed to be /-joined or system separator joined?
-