-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) If this is unnecessary, please remove it. If it will be uncommented in the future, please add a note explaining why it's turned off.
-
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) liua6_codeReview isn't something portable, is it?
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) No spaces between parentheses and function parameters.
-
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) Python makes it really easy to iterate like this: for line in lines: if not re.match("^\s*$", line): clearcase_diff_lines.append(line)
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) No parentheses around the while condition. You can also use the for ... in ... syntax to make this a lot more readable.
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) Indentation here looks like tabs. All indentation should be 4 spaces.
-
-
-
-
/trunk/reviewboard/scmtools/clearcase.py (Diff revision 1) This is just leftover from the SVN tool, no?
Add clearcase integration
Review Request #641 — Created Nov. 17, 2008 and discarded
Information | |
---|---|
aaron | |
Review Board SVN (deprecated) | |
trunk | |
current, no | |
Reviewers | |
reviewboard | |
clearcase integration. add a file clearcase.py under scmtools, add a record in file scmtools\fixtures\initial_data.json.
It works fine in our team.
-
Lots of little things that should be cleaned up. Some large things, but also a lot of code that can be simplified with some Pythonisms. I am wondering, though, why this converts from a Clearcase diff into a standard GNU diff instead of implementing a diff parser. It seems you should be adding a DiffParser subclass that does the work necessary to parse. What are the differences between Clearcase diffs and standard diffs? We may be able to simplify this code drastically.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) We can't put code in that's specific to a single platform and requires modification to make it work. What's the reason this can't be used on Windows?
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) How do these names work? I'm a little concerned about having a hard-coded name here.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) The file handle returned from open() must be close()'d or there will be a leak.
-
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Only a single blank line between these functions.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Should just return in each case instead of assigning to r.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) An empty filename here is confusing. I'm not even sure why this exception is here?
-
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) This is better done as: return DiffParser("\n".join(self.ccdiff_to_gnudiff(data)))
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Can you document exactly what this function is doing? It's kind of confusing.
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Indentation looks to be using tabs instead of spaces here.
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) This could be better written as: clearcase_diff_lines = [ line for line in lines if not re.match("^\s*$", line) ]
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) The first part of the if statement should always be true, since we've verified it through the condition in the while loop.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) When doing multi-line if statements, it's best to do: if (.... and .... and ....):
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Indentation is wrong for all this. It should be indented 4 spaces from the if statement.
-
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) These should use \t before the (revision ...) instead of spaces. Also, the hard-coded "(revision 100)" seems very bad to me. We need to use actual revisions, but made-up ones.
-
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) These might be better as: newline = "@@ -%s,0 +%s,%s @@" % (old_start + 1, new_start, new_line_num) Same with the other ones.
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) I would do something like: for i in range(i + 1, i + new_line_num): gnu_diff_lines.append("+" + clearcase_diff_lines[i][2:])
-
trunk/reviewboard/scmtools/clearcase.py (Diff revision 2) Similar to above: for i in range(i + 1, i + old_line_num): gnu_diff_lines.append("-" + clearcase_diff_lines[i][2:]) for i in range(i + 1, i + new_line_num): gnu_diff_lines.append("+" + clearcase_diff_lines[i][2:])