Bug 74253 – [Refactoring] Move top-level code to generate a new ChangeLog into a method

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Kentaro Hara]

The objective is to make prepare-ChangeLog a loadable Perl module for unit testing, which requires to remove top-level code and global variables. This bug is one of the incremental refactorings for that.

[Kentaro Hara]

Created attachment 118705 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 118705 [details]
Patch

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

r- for style issues with variable names, but otherwise this looks good.

> Tools/Scripts/prepare-ChangeLog:171
> +my $added_regression_tests = generateFileList(@changed_files, @conflict_files, %function_lists);

The $aded_regression_tests variable name does not match the prevalent style for variable names in other Perl scripts (and Python and C++ code).  I would expect it to be $addedRegressionTests.

Why is this style being used for these changes?

> Tools/Scripts/prepare-ChangeLog:453
> +    my ($prefixes, $files_in_change_log, $added_regression_tests, $function_lists) = @_;

CamelCase should be used for variables instead of underscores.

> Tools/Scripts/prepare-ChangeLog:1561
> +    my @added_regression_tests;

Should be @addedRegressionTests.

> Tools/Scripts/prepare-ChangeLog:1614
> +                push @added_regression_tests, $file

Ditto.

> Tools/Scripts/prepare-ChangeLog:1631
> +    return \@added_regression_tests;

Ditto.

[Kentaro Hara]

Created attachment 118707 [details]
Patch

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 118705 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118705&action=review
> 
> r- for style issues with variable names, but otherwise this looks good.
> 
> > Tools/Scripts/prepare-ChangeLog:171
> > +my $added_regression_tests = generateFileList(@changed_files, @conflict_files, %function_lists);
> 
> The $aded_regression_tests variable name does not match the prevalent style for variable names in other Perl scripts (and Python and C++ code).  I would expect it to be $addedRegressionTests.
> 
> Why is this style being used for these changes?

Fixed it.

Actually, I have been so far confused about the convention of variable names in Python and Perl scripts. (i.e. whether the WebKit coding style guide is applied to Python and Perl or not) because we can find many existing Python and Perl scripts that are using under_score_names. In case of ./prepare-ChangeLog, under_score_names appear to be being used a bit more than CamelCaseNames.

[David Kilzer (:ddkilzer)]

Comment on attachment 118707 [details]
Patch

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

r=me with the subroutine name change.

> Tools/Scripts/prepare-ChangeLog:451
> +sub generateNewChangeLog($$$\%)

This should probably be named generateNewChangeLogs() since it creates more than one.

[Kentaro Hara]

Created attachment 118708 [details]
patch for commit

[WebKit Review Bot]

Comment on attachment 118708 [details]
patch for commit

Clearing flags on attachment: 118708

Committed r102537: <http://trac.webkit.org/changeset/102537>

[David Kilzer (:ddkilzer)]

Manually marking as RESOLVED/FIXED.