-
-
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.
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) Should compile this once as a class-global declaration.
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) Can you put in a more human-readable error? This will be presented to users.
-
-
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) This is used several times. Can you declare this as part of the class and reference it?
-
-
-
-
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.
-
-
-
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'.
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) Multi-line conditionals should be contained in parens.
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) Should use splitlines() unless it's really important that it's specifically '\n'
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 1) This may be best as a comment, rather than part of the class documentation.
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
-
Add Plastic SCM support
Review Request #1395 — Created Feb. 8, 2010 and submitted
Information | |
---|---|
dick | |
Review Board | |
Reviewers | |
reviewboard | |
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.
-
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.
DI
Change Summary:
Uploaded diff again. This is the same as the previous diff, but hopefully it's viewable this time. Apologies for taking so long!
Diff: |
Revision 3 (+319 -1) |
---|
-
-
reviewboard/scmtools/plastic.py (Diff revision 3) Another inconsistency in our codebase, but constants like these should be all uppercase.
-
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..
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 3) Might help for debugging purposes to include the regex inline, and the data we tried to match.
-
reviewboard/scmtools/plastic.py (Diff revision 3) Probably don't need this past the initial development stages, I'd imagine?
-
reviewboard/scmtools/plastic.py (Diff revision 3) Alignment problem with the "(file_str". Should be aligned with the 'Plastic
-
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.
-
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?
-
reviewboard/scmtools/plastic.py (Diff revision 3) Should probably be super(PlasticDiffParser, self).__init__(data)
-
-
-
-
reviewboard/scmtools/plastic.py (Diff revision 3) Do you need to quote the repo? Popen *should* be doing proper quoting.
-