Bug 21222

Summary:

Formatted Diff for attachment 23920 is mangled

Product:

WebKit

Reporter:

Mark Rowe (bdash) <mrowe>

Component:

Tools / Tests

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, aroben, cmarcelo, commit-queue, dbates, eric

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

URL:

https://bugs.webkit.org/attachment.cgi?id=23920&action=prettypatch

Attachments:

Description	Flags
patch	aroben: review-
patch v2, moved normalization to lower layer	none

Description Mark Rowe (bdash) 2008-09-29 15:00:20 PDT

The formatted diff for attachment 23920 [details] is mangled.  The raw diff is at <https://bugs.webkit.org/attachment.cgi?id=23920&action=view>, with the mangled formatting at <https://bugs.webkit.org/attachment.cgi?id=23920&action=prettypatch>.

Comment 1 Mark Rowe (bdash) 2008-10-01 18:48:10 PDT

*** Bug 21291 has been marked as a duplicate of this bug. ***

Comment 2 Caio Marcelo de Oliveira Filho 2011-03-08 20:50:30 PST

Created attachment 85129 [details]
patch

Comment 3 Eric Seidel (no email) 2011-03-08 21:14:33 PST

Comment on attachment 85129 [details]
patch

Shouldn't this be done at a lower layer?  Or shouldn't prettyify ASSERT that it has the proper line endings?

Comment 4 Eric Seidel (no email) 2011-03-08 21:14:59 PST

How do we unit test this?  (I'm not sure we have unit tests for the ruby code?)

Comment 5 Adam Roben (:aroben) 2011-03-09 05:23:04 PST

Comment on attachment 85129 [details]
patch

I agree with Eric that the line-ending normalization should happen in PrettyPatch itself. prettify.rb is just supposed to be a thin wrapper around PrettyPatch.

Comment 6 Adam Roben (:aroben) 2011-03-09 05:23:43 PST

(In reply to comment #4)
> How do we unit test this?  (I'm not sure we have unit tests for the ruby code?)

We don't have unit tests. It would be pretty easy to add some, though!

Comment 7 Caio Marcelo de Oliveira Filho 2011-03-09 11:04:50 PST

Created attachment 85197 [details]
patch v2, moved normalization to lower layer

Comment 8 Caio Marcelo de Oliveira Filho 2011-03-09 11:13:06 PST

(In reply to comment #6)
> We don't have unit tests. It would be pretty easy to add some, though!

What kind of tests are you thinking about, one that gets a set of patches to run prettify and compare with some expected files? Or something different?

Any guidance on where to put those is welcome too :)

Comment 9 Eric Seidel (no email) 2011-03-09 11:14:20 PST

(In reply to comment #8)
> (In reply to comment #6)
> > We don't have unit tests. It would be pretty easy to add some, though!
> 
> What kind of tests are you thinking about, one that gets a set of patches to run prettify and compare with some expected files? Or something different?
> 
> Any guidance on where to put those is welcome too :)

I was thinking more along the lines of the python unit tests:
*_unittest.py under http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/

Comment 10 WebKit Commit Bot 2011-03-10 16:57:03 PST

Comment on attachment 85197 [details]
patch v2, moved normalization to lower layer

Clearing flags on attachment: 85197

Committed r80795: <http://trac.webkit.org/changeset/80795>

Comment 11 WebKit Commit Bot 2011-03-10 16:57:07 PST

All reviewed patches have been landed.  Closing bug.