Should be a blank line separating these. djblets is a third-party module, so it's in the second grouping of imports.

Can you make all the imported things align?

Trailing whitespace.

Should compile this once as a class-global declaration.

Excess blank line.

Can simplify these lines to:

    if line:

Can you put in a more human-readable error? This will be presented to users.

Same here.

Trailing whitespace.

Same here.

Same here.

This is used several times. Can you declare this as part of the class and reference it?

Trailing whitespace.

And here.

And here.

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.

Only one blank line.

Same comment about the description.

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'.

No parens.

Multi-line conditionals should be contained in parens.

Trailing whitespace.

Should use splitlines() unless it's really important that it's specifically '\n'

Should be on the class as well.

"post-review"

This may be best as a comment, rather than part of the class documentation.

BINARY_RE

Can assign m.group(2) directly. No need for '%s'

Should use parens for multi-line conditionals.

Should inherit from object.

Trailing whitespace.

Here too.

And here.

And here.

Should remove this.

No need for parens.

Trailing whitespace.

And here.

And here.

And here.

No need for parens.

No need for parens.

Should remove these lines.

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.

Another inconsistency in our codebase, but constants like these should be all uppercase.

This should be: super(PlasticTool, self).__init__(repository)

We don't do this everywhere yet, but we should start moving in that direction..

No space between function and (

No need for \

Might help for debugging purposes to include the regex inline, and the data we tried to match.

Probably don't need this past the initial development stages, I'd imagine?

Alignment problem with the "(file_str". Should be aligned with the 'Plastic

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.

Out of curiosity, any plans to add some sort of indicators to the native diff output from the Plastic client?

Should probably be super(PlasticDiffParser, self).__init__(data)

No need for \

Is repserver always used for Plastic repo URIs?

No need for the trailing \

Do you need to quote the repo? Popen *should* be doing proper quoting.

Same comments about quoting and trailing \

At this point, things look good. Thanks so much!