Removing extraordinary whitespace at end of lines is obvious for me so :) don't waste time to this :)

I see this :) next time will be fixed :D

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.

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.

Doc strings should have the form of:

"""One line summary.

Any remaining documentation.
"""

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

No blank line between the docstrings and the code.

"your", not "Your".

What happens if you're in the source tree for a non-Clear Case repository? Will this die still? It doesn't look safe.

"Get the properties of the view"

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: ")
]

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?

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

"Webviews are not supported." instead of "We don't support webviews."

Blank line before the next comment.

Not sure this comment adds any value. The code is self-explanatory.

"To generate a diff using a parent branch or by passing revision ranges, you must use a dynamic view."

Should follow the docstring pattern mentioned above.

I don't understand this at all. Why are we using infinity?

Blank line between these.

No comma after version.

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

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)

Combine to one line.

I don't really understand what this means?

Blank line between these.

No blank line here.

Also, 'type' is a reserved word. Should use a different variable here.

debug("Skipping compressed file %s." % path)

Blank line between these.

if path not in changelist:

Should combine this:

changelist[path] = {
    'highest': ...
    ...
}

No parens.

No trailing comas.

The indentation is messed up. Params should always align. Maybe:

changeranges.append(self._construct_extended_path(
    path, version['previous']))

One line.

Same about the blank lines, 'type' variable, and debug output.

No trailing comma.

Make sure these fit in 80 columns.

One line.

"Return the content of the object in the specified path."

Use 'f' instead of 'file'. 'file' is a reserved keyword.

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.

"""Return information about the checked out changeset."""

And then maybe something about what type of data is returned.

No blank line.

"We ignore return code 1 in order to omit files that Clear Case can't read."

All parameters should be aligned, and only one parameter per line.

Blank line between these.

Blank line between these.

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

No blank line after the docstring, but there should be one between 'changeset = []' and the comments.

Should be changed to what I mentioned above.

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?

Align all parameters, and only one parameter per line.

Also, for readability, each parameter to cleartool should also be on its own line.

Blank line between these.

This is duplicated in two places. Probably should be pulled out into a common function.

One line.

No need to define this, since it'll be defined to something regardless.

Blank line between these.

Add a period after the sentence.

Also, should be one line.

"Performs a diff between the passed revisions from the Clear Case repository."

No blank line here.

Add a period. And should be one line.

"Generates a unified diff for all files in the changeset."

No blank line here.

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.

These can be combined into one statement.

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 :)

Most of this looks pretty good. Is this at a state where you want it committed?

Can you align this with the beginning of the list?

Trailing whitespace.

Indent the second line 1 more space.

Trailing whitespace.

Trailing whitespace.

Why the move?

Imports should be alphabetical, so undo this change.

Two blank lines between classes; undo this change.

Delete this blank line.

Align "-cview" with "cleartool"

I think it'll actually be more efficient to swap these:

properties = line.split(' ')
if properties[0] == 'Properties:':

Can you figure out which of our open clearcase bugs are fixed by this change?

Two blank lines between classes.

whitespace.

whitespace.

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.

Pushed to master as 47fb2ce. Thanks!