[RBTools] Brand new Clear Case implementation
Review Request #1843 — Created Oct. 18, 2010 and submitted
This is new, wrote from scratch Clear Case implementation which provide new functionality and fix old issues: * recognize view type * remove necessity of using xargs trick to send review * pre-review all CHECKEDOUT files in view - just run "post-review" command * post-review all changes made on branch passed as --tracking-branch * post-review custom revision range passed as --revision-range * remove useless --label option * simple command "post-review" at least generate diff of CHECKEDOUT files * fixed "\No new line at end of file" workaround * diffs generate properly under windows * no cygwin required * Make use of diff instead broken unified_diff Python implementation where it is important * Handle binary files * Updated documentation
Linux: * Tested Windows: * Need tests after changes
Change Summary:
Missing word "expert"
Description: |
|
---|
Change Summary:
* diffs generate properly under windows * strip to 80 characters columns width * remove extraordinary whitespaces
Diff: |
Revision 2 (+238 -282) |
---|
Change Summary:
Update description
Description: |
|
---|
Change Summary:
Update TODO section
Description: |
|
---|
-
-
rbtools/postreview.py (Diff revision 3) I'm confused. I can't understand role of path and base_path parameter. They cause that on ReviewBoard side i get relative or absolute path but I don't know how use it to get correct result.
Change Summary:
Windows compatible implementation
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Diff: |
Revision 4 (+251 -281) |
Change Summary:
Extend TODO
Description: |
|
---|
-
Hey Jan, Thanks for the change! There's a bunch of stylistic things to fix, and a few small logic things. Most of this review really boils down to both style and English grammar, which I'm happy to help with as I understand more about what the functions do.
-
rbtools/postreview.py (Diff revision 6) Doc strings should have the form of: """One line summary. Any remaining documentation. """
-
rbtools/postreview.py (Diff revision 6) Should be: """Returns information on the Clear Case repository. This will first check if the cleartool command is installed and in the path, and post-review was run from inside of the view. """ (Word-wrap that so nothing goes over 80 columns. Same with any other examples I give.)
-
-
-
rbtools/postreview.py (Diff revision 6) What happens if you're in the source tree for a non-Clear Case repository? Will this die still? It doesn't look safe.
-
-
rbtools/postreview.py (Diff revision 6) This looks kind of odd to me. Is this meant to be one big statement? I'd prefer the result of execute() be computed first, and then passed to the filter. Actually, filter shouldn't really be used anymore. Instead, do: property_lines = execute(...) properties_line = [ line for line in property_lines if line.startswith("Properties: ") ]
-
rbtools/postreview.py (Diff revision 6) Oh, if we only ever use the first line, why not just use a for loop for the above, and then when we find that line, we do the calculations and break?
-
rbtools/postreview.py (Diff revision 6) # Determine the view type and check if it's supported. # # Specifically check if webview was listed in properties # because webview types also list the 'snapshot' # entry in properties.
-
rbtools/postreview.py (Diff revision 6) "Webviews are not supported." instead of "We don't support webviews."
-
-
rbtools/postreview.py (Diff revision 6) Not sure this comment adds any value. The code is self-explanatory.
-
rbtools/postreview.py (Diff revision 6) "To generate a diff using a parent branch or by passing revision ranges, you must use a dynamic view."
-
-
-
-
-
rbtools/postreview.py (Diff revision 6) """Returns a sanitized version the diff. This will ensure that the diff either ends with a newline or has the appropriate "No newline at end of file" marker. """
-
rbtools/postreview.py (Diff revision 6) This will actually be more efficient if you do: fixed_diff = list(diff) if not fixed_diff[-1].endswith('\n'): fixed_diff.append('\n\ No newline at end of file\n' return ''.join(fixed_diff)
-
-
-
-
rbtools/postreview.py (Diff revision 6) No blank line here. Also, 'type' is a reserved word. Should use a different variable here.
-
-
-
-
rbtools/postreview.py (Diff revision 6) Should combine this: changelist[path] = { 'highest': ... ... }
-
-
rbtools/postreview.py (Diff revision 6) No trailing comas. The indentation is messed up. Params should always align. Maybe: changeranges.append(self._construct_extended_path( path, version['previous']))
-
-
rbtools/postreview.py (Diff revision 6) Same about the blank lines, 'type' variable, and debug output.
-
-
-
-
-
rbtools/postreview.py (Diff revision 6) You can check for this in order to have a more specific error message by doing: elif os.access(extended_path, os.R_OK): Also, %s instead of %r, and no need to wrap extended_path in parens.
-
rbtools/postreview.py (Diff revision 6) """Return information about the checked out changeset.""" And then maybe something about what type of data is returned.
-
-
rbtools/postreview.py (Diff revision 6) "We ignore return code 1 in order to omit files that Clear Case can't read."
-
rbtools/postreview.py (Diff revision 6) All parameters should be aligned, and only one parameter per line.
-
-
-
rbtools/postreview.py (Diff revision 6) The wrapping is sort of strange. Make sure doc strings are wrapped to just under 80 columns. Please change to: """Returns information about the versions changed on a branch. This takes into account the changes on the branch owned by the current user in all vobs of the current view. """
-
rbtools/postreview.py (Diff revision 6) No blank line after the docstring, but there should be one between 'changeset = []' and the comments.
-
-
rbtools/postreview.py (Diff revision 6) Is this so the environment variable will be calculated in the command line? If so, can we just get it ourselves using os.getenv and then pass that into the command line?
-
rbtools/postreview.py (Diff revision 6) Align all parameters, and only one parameter per line. Also, for readability, each parameter to cleartool should also be on its own line.
-
-
rbtools/postreview.py (Diff revision 6) This is duplicated in two places. Probably should be pulled out into a common function.
-
-
rbtools/postreview.py (Diff revision 6) No need to define this, since it'll be defined to something regardless.
-
-
-
rbtools/postreview.py (Diff revision 6) "Performs a diff between the passed revisions from the Clear Case repository."
-
-
-
-
-
rbtools/postreview.py (Diff revision 6) This really should be: for previous, current in changeset: And then when building this, put previous and current together in a tuple, instead of their own entries.
-
Change Summary:
Decrease todo to reasonable numer of changes.
Description: |
|
---|
-
Hi Christina :) Thank You for Your points and ideas :] Some of them just change my way of thinking about Python. I fix the code as You said. I hope this will be the last time before You accept this patch. I made fixes for Windows :) so there is some changes (which I should also include in documentation). I checked is this patch works also under Windows and it looks like on both systems patches are generating fine. I thought I will write implementation using oid/uuid ids but I don't have time for that now. This is something for next year :) but I don't know I will do it. There is a little hope there is someone new to this. I have removed TODOs from code and raise new tickets on issue list :) I believe this is a good way to remember what will be nice to have. IMHO this implementation is fair enough to be useful :) I hope I get right :) I'm looking forward Your review :) Happy New Year! and See You in 2011 :)
Change Summary:
Remove TODO and FIXME from description
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Testing Done: |
|
Change Summary:
* Add timestamp to diff * Default .reviewboardrc is good enough (tested) * Use --tracking-branch parameter instead --parent which is more accurately
Description: |
|
||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Bugs: |
|
||||||||||||||||||||||||||||||||||||
Diff: |
Revision 9 (+277 -286) |
Change Summary:
Please review this implementation. After final fixes I will update documentation and it will be ready to release.
Description: |
|
||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+288 -287) |
Change Summary:
Remove trailing whitespaces Add update of documentation
Description: |
|
|||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+288 -287) |
-
-
-
-
-
-
rbtools/postreview.py (Diff revision 11) I think it'll actually be more efficient to swap these: properties = line.split(' ') if properties[0] == 'Properties:':
Change Summary:
Fix multiplatform issues for ClearCase implementation.
Diff: |
Revision 12 (+360 -288) |
---|
-
If I didn't done something stupid it is ready to release for me. This should be backward compatible - but I don't give my head for this. I hope fix will be fast :) I don't like keep my own version of ReviewBoard on branch.
Change Summary:
Fix some more documentation changes and reviewers idea.
Diff: |
Revision 13 (+372 -288) |
---|