Add Plastic SCM support

Review Request #1395 — Created Feb. 8, 2010 and submitted

Information

Review Board

Reviewers

Implemented support for the Plastic SCM server.  http://www.codicesoftware.com

Supports changesets, and branch diffs.
Tested (with post-review generating the diff file) against the latest version of Plastic SCM; also with post-review directly.
chipx86
  1. 
      
  2. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
    Should be a blank line separating these. djblets is a third-party module, so it's in the second grouping of imports.
  3. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
     
     
    Can you make all the imported things align?
  4. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  5. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
    Should compile this once as a class-global declaration.
  6. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
    Excess blank line.
  7. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
    Can simplify these lines to:
    
        if line:
  8. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Can you put in a more human-readable error? This will be presented to users.
  9. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Same here.
  10. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  11. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Same here.
  12. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Same here.
  13. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    This is used several times. Can you declare this as part of the class and reference it?
  14. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  15. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  16. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  17. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
     
    The docs should first describe the function. Part of this description should contain the format that's being parsed, but that shouldn't be in the summary.
  18. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
     
    Only one blank line.
  19. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
     
    Same comment about the description.
  20. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
     
     
     
    Would be nice to keep all this actual 'cm' execution code actually in the PlasticClient class and call out to it. Ideally, PlasticTool won't need to know anything about 'cm'.
  21. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    No parens.
  22. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
    Multi-line conditionals should be contained in parens.
  23. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  24. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Should use splitlines() unless it's really important that it's specifically '\n'
  25. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Should be on the class as well.
  26. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    "post-review"
  27. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
    This may be best as a comment, rather than part of the class documentation.
  28. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    BINARY_RE
  29. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Can assign m.group(2) directly. No need for '%s'
  30. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
     
    Should use parens for multi-line conditionals.
  31. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Should inherit from object.
  32. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  33. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Here too.
  34. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  35. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  36. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Should remove this.
  37. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    No need for parens.
  38. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    Trailing whitespace.
  39. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  40. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  41. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    And here.
  42. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    No need for parens.
  43. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
    No need for parens.
  44. reviewboard/scmtools/plastic.py (Diff revision 1)
     
     
     
    Should remove these lines.
  45. 
      
DI
chipx86
  1. I've been unable to view this diff. It seems that the diff doesn't apply. Can you try uploading it again? I'd love to get this in for the upcoming beta.
  2. 
      
DI
chipx86
  1. 
      
  2. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
     
     
     
    Another inconsistency in our codebase, but constants like these should be all uppercase.
  3. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
    This should be: super(PlasticTool, self).__init__(repository)
    
    We don't do this everywhere yet, but we should start moving in that direction..
  4. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    No space between function and (
  5. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    No need for \
  6. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    Might help for debugging purposes to include the regex inline, and the data we tried to match.
  7. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
    Probably don't need this past the initial development stages, I'd imagine?
  8. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
    Alignment problem with the "(file_str". Should be aligned with the 'Plastic
  9. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    It is. It should probably be renamed at some point, though.
    
    PRE_CREATION really means "new files." So any referenced file being saved to the database that is actually a new, uncommitted file should be normalized to have this revision.
    
    The diff viewer will then use that to indicate if it's a new, uncommitted (upstream) file.
  10. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
     
    Out of curiosity, any plans to add some sort of indicators to the native diff output from the Plastic client?
  11. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    Should probably be super(PlasticDiffParser, self).__init__(data)
  12. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    No need for \
  13. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    Is repserver always used for Plastic repo URIs?
  14. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
     
    No need for the trailing \
  15. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
    Do you need to quote the repo? Popen *should* be doing proper quoting.
  16. reviewboard/scmtools/plastic.py (Diff revision 3)
     
     
     
     
     
     
    Same comments about quoting and trailing \
  17. 
      
DI
Review request changed

Change Summary:

New diff, incorporating all requested changes

Diff:

Revision 4 (+317 -1)

Show changes

david
  1. At this point, things look good. Thanks so much!
  2. 
      
Loading...