Bug 130508 – prepare-ChangeLog should list functions that have been removed too.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[László Langó]

The prepare-ChangeLog does not list the deleted functions in the Changelog. We have to read the functions and the line ranges of them from the source before the change, then search for overlaps between them and the changed line ranges from diff.

[László Langó]

Created attachment 227287 [details]
Patch

[Alexey Proskuryakov]

On one hand, information about which functions were removed seems as relevant as information about added functions. But on the other hand, the list of functions in ChangeLog is just a template for adding per-function comments manually, and there is rarely anything to say about deleted functions ("deleted because this function is no longer needed, duh").

Overall, I'm leaning slightly against this proposed change.

[László Langó]

(In reply to comment #2)
> On one hand, information about which functions were removed seems as relevant as information about added functions. But on the other hand, the list of functions in ChangeLog is just a template for adding per-function comments manually, and there is rarely anything to say about deleted functions ("deleted because this function is no longer needed, duh").
> 
> Overall, I'm leaning slightly against this proposed change.

I think this is a useful information. A few days ago I had to find a missing method, but I could not find out from the git blame info and neither from the ChangeLog. I wanted to know that when was it removed and why. This why I started to work on this fix.

There is a TODO list in the prepare-ChangeLog, that contains this change. If we don't want this feature, then we should remove it from the list:
http://trac.webkit.org/browser/trunk/Tools/Scripts/prepare-ChangeLog#L38

[Darin Adler]

I like having the list of deleted functions. The person making the change log can always edit them out if they are not helpful.

[Darin Adler]

Comment on attachment 227287 [details]
Patch

Despite Alexey’s concerns, I like doing this. If it really gets in the way we can put it behind an option.

But don’t we have some sort of regression test system for prepare-ChangeLog? I seem to remember there was one. This should be covered by tests in that system if we have it.

[László Langó]

(In reply to comment #5)
> (From update of attachment 227287 [details])
> Despite Alexey’s concerns, I like doing this. If it really gets in the way we can put it behind an option.
> 
> But don’t we have some sort of regression test system for prepare-ChangeLog? I seem to remember there was one. This should be covered by tests in that system if we have it.

Not exactly for the prepare-ChangeLog, but only for the parser functions in it. Because we need a local change on source files, it's hard to write a unittest.

[László Langó]

Created attachment 227630 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 227630 [details]
patch for landing

Clearing flags on attachment: 227630

Committed r166156: <http://trac.webkit.org/changeset/166156>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.