Bug 74253 - [Refactoring] Move top-level code to generate a new ChangeLog into a method
Summary: [Refactoring] Move top-level code to generate a new ChangeLog into a method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-11 01:20 PST by Kentaro Hara
Modified: 2011-12-12 08:50 PST (History)
4 users (show)

See Also:


Attachments
Patch (9.30 KB, patch)
2011-12-11 01:39 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
Patch (11.85 KB, patch)
2011-12-11 07:36 PST, Kentaro Hara
ddkilzer: review+
Details | Formatted Diff | Diff
patch for commit (11.85 KB, patch)
2011-12-11 08:25 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2011-12-11 01:20:14 PST
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.
Comment 1 Kentaro Hara 2011-12-11 01:39:01 PST
Created attachment 118705 [details]
Patch
Comment 2 David Kilzer (:ddkilzer) 2011-12-11 07:22:03 PST
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.
Comment 3 Kentaro Hara 2011-12-11 07:36:05 PST
Created attachment 118707 [details]
Patch
Comment 4 Kentaro Hara 2011-12-11 07:36:15 PST
(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.
Comment 5 David Kilzer (:ddkilzer) 2011-12-11 08:15:16 PST
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.
Comment 6 Kentaro Hara 2011-12-11 08:25:59 PST
Created attachment 118708 [details]
patch for commit
Comment 7 WebKit Review Bot 2011-12-11 10:05:07 PST
Comment on attachment 118708 [details]
patch for commit

Clearing flags on attachment: 118708

Committed r102537: <http://trac.webkit.org/changeset/102537>
Comment 8 David Kilzer (:ddkilzer) 2011-12-12 08:50:33 PST
Manually marking as RESOLVED/FIXED.