On Fri 06 Oct 2006 at 04:14PM, David Bustos wrote:
Ok, I've processed your webrev.sh comments. Thanks for the thorough
look.
-dp
> tools/scripts/webrev.sh
> HTML= & FRAMEHTML=: These seem mostly the same. Can we combine them
> with printf(1) or such?
These are two hard coded strings... I don't see the value in replacing
them with code...
> 76: This semicolon is unnecessary, isn't it?
Trailing semicolons are not strictly necessary for the last rule
in a CSS rule block, but I prefer them-- if you ever add another
clause, it's easy to forget to add the semicolon.
> 129: Is the font-family necessary? Won't most browsers render
> preformatted elements monospaced by default? And why are you
> setting the font size explicitly?
I did a bunch of experimentation with firefox's print engine.
This was what I came up with to get the output to look reasonable
on the printers to which I have access...
> 154,164,174,585,636: Can you make these input_cmd & output_cmd?
Done.
> 154: Lines 428 & 431 also do "html_quote input_file | output_cmd".
I updated the comment for html_quote().
> 199: Is this paragraph really necessary?
No-- It was from the original. I deleted it.
> 202: I think this sentence can be coalesced with the introductory
> paragraph.
Done.
> 212s/changes/changed/
Fixed.
> sdiff_to_html(): Would this be any simpler in perl?
Probably, but I didn't write it, and I don't want to mess with it.
If I had it to do over, I might have rewritten the whole thing in perl,
but until we undertake a true 'webrev v2', I didn't want to. Another
incremental improvement would be to pull the 'diff' parts out into a
tool (in perl) which parallels 'wdiff' (or expand wdiff...), leaving
webrev.sh to manage the overall process.
> 257-8: It appears you're adding arguments. You should probably
> document them in the preceding comment.
Thanks. Done.
> 346-57: These lines are remarkably similar to 264-74. Did you
> consider combining them?
No, I haven't... this sort of follows from my previous comment.
> 427,430: Should /tmp/sN be prefixed with $$?
Thanks-- I think /tmp/s1 and /tmp/s2 were there for my debugging
purposes. I removed them entirely.
> 454: Does this need to be before the </body></html> line? It looks
> like framed_sdiff directs all of its output to another file.
Good point.
> framed_sdiff: This comment should probably say where all of the
> content is placed.
Fixed.
> 482: It seems like this might be better done with a heredoc.
Fixed.
> 479: Would this be better accomplished with <BASE>?
AFAIK, <BASE> requires that you know the absolute path to the content
(i.e. http://cr.grommit.com/~dp/foo/bar/). Since webrevs are designed
to be relocatable, I'm not sure how <BASE> would help.
> 518: Does this not work with EOF indented? ksh(1) indicates that it
> should, with <<-.
> 525: "Merge concatenate" seems like a grammatical error.
> 537: This seems unused.
> 554: text or what?
All fixed.
> 590: Shouldn't this be "a horizontal rule"?
Probably depends upon which country you grew up in. Fixed.
> 596: I think you have invalidated this comment about color & font.
Fixed.
> strip_unchanged():
> - Can we put this closer to sdiff_to_html(), which is the only place
> it's used?
>
> - Wouldn't it be a lot easier to take the diff -u output and blank
> the + lines for the left side and blank the - lines for the right
> side? I suppose you'd have to do some computation for changed
> lines.
Possibly--- again, I left this code alone.
> 645: You invalidated the font color portion of this comment.
> 694s/relative_cws/relative_dir/
> 1020-5: Would this be neater as a heredoc?
All fixed.
> 1066:
> - I think it would be clearer if you passed $1 & $2 to html_quote.
Ok.
> - Didn't you change other files to use four columns for the line
> number instead of three?
Indeed-- fixed.
> 1378: You could reduce the indentation in this function by doing
>
> [[ -r $FLIST ]] || return
Fixed.
> 1439,1444: These descriptions are capitalized, but the rest are not.
Thanks, fixed.
> 1522-3: Why not combine these conditions?
Fixed.
> 1751: If mkdir fails, won't it complain?
Good point.
> 1766-7: I wonder if you should be using printf for this, so it will be
> easy to adjust the indentation.
I'll look into it.
> 1883-4: I think you can do this with [[ $pclosed = usr/closed/* ]], or
> with a case statement.
Cool. Fixed.
> 1894: Do you mean absolute paths in the output of diff?
Yes... in the diffs we now have:
--- old/usr/src/tools/scripts/webrev.sh
+++ new/usr/src/tools/scripts/webrev.sh
Whereas in the old version it would put the absolute paths.
> 1910: Blank line, please.
> 1913: Why are you using /bin/rm here, but not elsewhere?
> 1957: Isn't this redundant with 1953?
> 2113: This doesn't seem like good English.
All fixed.
> 2117: I think you can do this with
>
> userupper=`perl -e "print ucfirst $username"`
Cool!
> 2262: This change seems unnecessary.
I got the feedback from Bill Shannon that he uses the /usr/ucb version
of 'echo' (!)-- since /usr/ucb is first in his path-- and that he was having
issues with \c characters in echo statements. So for consistency, I
altered all echo statements in the program to print statements.
> 2279: This seems like it's missing something.
I'm not understanding this clue. What is missing?
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
tools-discuss mailing list
tools-discuss@opensolaris.org
