Bug 73788 – In prepare-ChangeLog, move top level code into prepareChangeLog()

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Kentaro Hara]

This is one of the steps to make prepare-ChangeLog a loadable Perl module (bug 73531).

We should create a new subroutine prepareChangeLog(), and move top level code into prepareChangeLog().

[Kentaro Hara]

Created attachment 117816 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117816 [details]
Patch

Attachment 117816 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10732508

New failing tests:
svg/custom/linking-uri-01-b.svg

[Kentaro Hara]

Created attachment 118030 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 118030 [details]
Patch

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

I think this function is way too big. We should put each block into a small function first. As is, the patch is quite in-comprehensive and it'll mess up the svn blame quite a lot.

> Tools/Scripts/prepare-ChangeLog:220
> +    if (%changed_line_ranges) {

You might consider putting this entire block in some function.

> Tools/Scripts/prepare-ChangeLog:233
> +          FUNCTION: foreach my $function_range_ref (@function_ranges) {

Wrong indentation.

[Kentaro Hara]

(In reply to comment #4)
> (From update of attachment 118030 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118030&action=review
> 
> I think this function is way too big. We should put each block into a small function first. As is, the patch is quite in-comprehensive and it'll mess up the svn blame quite a lot.

rniwa: Thank you for the review. I agree to splitting the big function, but I'd like to do it later with unit tests.

As ddlizker mentioned in bug 73531, our current objective is to change prepare-ChangeLog just enough to make it a loadable Perl module for unit testing. In other words, we should not make extra changes, e.g. refactoring like splitting the big function, to prepare-ChangeLog until we have unit tests for it.

[Kentaro Hara]

Based on discussions on IRC with rniwa, we decided to commit this patch more incrementally.

ddkilzer, aroben: rniwa is going to review these patches.