[Webkit-unassigned] [Bug 39170] Add support function to parse SVN single-line and multi-line property value
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sat May 15 20:10:37 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39170
--- Comment #5 from Daniel Bates <dbates at webkit.org> 2010-05-15 20:10:37 PST ---
(In reply to comment #3)
> (From update of attachment 56171 [details])
> Thanks, Dan! Comments below. r- to consider them:
>
> > Index: WebKitTools/ChangeLog
> > + Add function parseSvnPropertyPlusOrMinus to parse single-line and multi-line
> > + property values.
>
> Would parseSvnPropertyValue() be more direct?
Will change.
>
> > +
> > + * Scripts/VCSUtils.pm:
> > + Added function parseSvnPropertyPlusOrMinus. We will use this as part of
> > + Bug #38885.
>
> It might be helpful to include a bug URL for clickability.
Will change.
>
> > Index: WebKitTools/Scripts/VCSUtils.pm
> >
> > my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
> > my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> > +my $svnPropertyChangeStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $1 is the name of the property
> > +my $svnPropertyValueRegEx = qr#^ (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).
>
> Would $svnPropertyValueStartRegEx be better since it is just the first line?
Will change.
>
> > +# Parse the property value of an SVN property from the given file handle, and advance
> > +# the handle so the last line read is the first line after the property value.
>
> I would abbreviate this to, "Parse the value of an SVN property change from..."
> and then in a separate paragraph include that an SVN property change is
> a section in the footer of an SVN diff with one or two property value
> sub-sections.
>
> > +# This subroutine dies if the first line is not a valid SVN property value line
> > +# (i.e. a line that begins with " +" or " -").
>
> "...not the beginning of a valid..."
Will change.
>
> > +# $fileHandle: advanced so the last line read from the handle is the first
> > +# line of the property value to parse. This should be a line
> > +# beginning with " +" or " -".
>
> You already said this above. Probably just include it in one of the two
> places.
>
> > +# FIXME: This method is unused as of (05/15/2010). We will call this function
> > +# as part of parsing a property from the SVN footer. See Bug #38885.
>
> Maybe include bug URL.
>
> > +sub parseSvnPropertyPlusOrMinus($$)
> > +{
> > + my ($fileHandle, $line) = @_;
> > +
> > + $_ = $line;
> > +
> > + my $propertyValue;
> > + if (/$svnPropertyValueRegEx/) {
> > + $propertyValue = $2; # Does not include the end-of-line character(s)
> > + } else {
> > + die("Failed to find '+' or '-' property: \"$_\".");
>
> Maybe "Failed to find property value beginning with '+' or '-': ..."?
>
> > + }
> > +
> > + $_ = <$fileHandle>;
> > + $propertyValue .= $POSTMATCH if defined($_); # Preserve the end-of-line of the first line in a multi-line property value.
>
> It looks like you're including the end-of-line characters even if the
> property value has just one line. Also, could you possibly rework things
> so that the $POSTMATCH value is read from closer to the reg ex (e.g. right
> after where $propertyValue is set to $2?
>
> > +# The array of test cases.
>
> This is probably redundant (even though it's in the other files).
Will remove, note it appears in parseDiff.pl, parseGitDiffHeader.pl and parseSvnDiffHeader.pl.
>
> > +my @testCaseHashRefs = (
> > +{
> > + # New test
>
> Is it necessary to write "# New test" at the beginning of each test case?
I think it is easier to identify the test cases using this comment, although we should find a better way to demarcate the test cases, if possible.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list