Bug 52400 – add container divs for diff blocks

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

add container divs for diff blocks

[Ojan Vafai]

Created attachment 78860 [details]
Patch

[Ojan Vafai]

I don't really know ruby at all, so don't hesitate to let me know if I'm doing something in a stupid way.

[Adam Barth]

Comment on attachment 78860 [details]
Patch

The JavaScript parts look good to me.  The ruby parts are mysterious.

[Ojan Vafai]

Created attachment 78864 [details]
Patch

[Adam Roben (:aroben)]

Comment on attachment 78860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78860&action=review

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:611
> +            @blocks = []
> +            diff_block_part = nil

It's strange that you declare diff_block_part here but not diff_block.

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:614
> +            for index in 1...lines.length
> +                line = lines[index]

A more idiomatic way of doing this is:

lines[1...lines.length].each do |line|

or:

for line in lines[1...lines.length]

> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:619
> +                    if (!diff_block_part or (diff_block_part.className != 'remove' and diff_block_part.lines.length != 0))

It's more idiomatic to say diff_block_part.nil? instead of !diff_block_part. Similarly, !diff_block_part.lines.empty? is better than diff_block_part.lines.length != 0.

How would we end up with a DiffBlockPart that has no lines?

[Ojan Vafai]

Created attachment 78867 [details]
Patch

[Ojan Vafai]

Comment on attachment 78860 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78860&action=review

>> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:611
>> +            diff_block_part = nil
> 
> It's strange that you declare diff_block_part here but not diff_block.

Declared both. In practice, we always create a diff_block before accessing it, but this seems cleaner anyways.

>> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:614
>> +                line = lines[index]
> 
> A more idiomatic way of doing this is:
> 
> lines[1...lines.length].each do |line|
> 
> or:
> 
> for line in lines[1...lines.length]

Did the latter.

>> Websites/bugs.webkit.org/PrettyPatch/PrettyPatch.rb:619
>> +                    if (!diff_block_part or (diff_block_part.className != 'remove' and diff_block_part.lines.length != 0))
> 
> It's more idiomatic to say diff_block_part.nil? instead of !diff_block_part. Similarly, !diff_block_part.lines.empty? is better than diff_block_part.lines.length != 0.
> 
> How would we end up with a DiffBlockPart that has no lines?

Used nil? and removed the lines length checks. That was a leftover from a previous iteration that happened to be correct (whoops!).

[Adam Barth]

Comment on attachment 78867 [details]
Patch

aroben said he was happy with the ruby and I'm happy with the JS.  Let's rock and roll.

[Ojan Vafai]

Committed r75747: <http://trac.webkit.org/changeset/75747>