[Webkit-unassigned] [Bug 148498] Modify prepare-Changelog to be aware of files that are marked as tests as well as files that are marked as requiring corresponding tests.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=148498

--- Comment #8 from Jason Marcell <jmarcell at apple.com> ---
Comment on attachment 259995
  --> https://bugs.webkit.org/attachment.cgi?id=259995
Patch

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

>> Tools/ChangeLog:4
>> +        that are marked as requiring corresponding tests.
> 
> The second line seems like duplicate text that can be removed.

I addressed this.

>> Tools/ChangeLog:11
>> +        simulate Git's attribute behevaior.
> 
> Note that there are unit tests for prepare-ChangeLog in Tools/Scripts/webkitperl/prepare-ChangeLog_unittest/.
> 
> This patch didn't modify any methods that are already tested, but we want to make sure the tests still run with this change (because each unit test imports prepare-ChangeLog as a custom Perl module, so things like global variables can break that technique).
> 
> The tests are run thusly:  ./Tools/Scripts/test-webkitperl
> 
> Looks like there are a couple of tests that are broken now, but the prepare-ChangeLog tests look fine.

I ran the tests before and after my patch and diffed the results. I didn't see any change.

>> Tools/Scripts/prepare-ChangeLog:1815
>> +        my (@dirparts) = File::Spec->splitdir($file);
> 
> @dirparts should be named @dirParts or @directoryParts.

I addressed this.

>> Tools/Scripts/prepare-ChangeLog:1827
>> +            my $command = SVN . " propget $attr $subPath | tr -d '\\n'";
> 
> Any reason you use 'tr' here instead of chomp()?  (Not necessary to fix to land the patch.)

When I was developing on the command line I was using standard Unix tools. I meant to convert to native PERL upon incorporating this code into the code base. As such I've changed this to use chomp in a later patch.

>> Tools/Scripts/prepare-ChangeLog:2032
>> +                push @requiresTests, $file
> 
> I find returning string results of 'true' or 'unspecified' kind of unusual.  Is there a reason you couldn't use 1 for 'true' and '0' for unspecified?
> 
> Oh, is this to make the svn and git attributes more readable in that you're literally storing 'true' instead of just '1'?
> 
> (This doesn't need to be addressed before landing the change, but I'd like to understand the design decision here.)

I decided to work with strings internally but have attributeCommand return an integer (1 for true, 0 for false). Users should store a "1" for a given SVN property or Git attribute to indicate true.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150831/28f61d7a/attachment.html>