| Summary: | prepare-ChangeLog should list functions that have been removed too. | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | László Langó <llango.u-szeged> | ||||||
| Component: | Tools / Tests | Assignee: | László Langó <llango.u-szeged> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | ap, commit-queue, darin, gyuyoung.kim, rniwa | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=137889 | ||||||||
| Attachments: |
|
||||||||
|
Description
László Langó
2014-03-20 06:39:43 PDT
Created attachment 227287 [details]
Patch
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.
(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 I like having the list of deleted functions. The person making the change log can always edit them out if they are not helpful. 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.
(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. Created attachment 227630 [details]
patch for landing
Comment on attachment 227630 [details] patch for landing Clearing flags on attachment: 227630 Committed r166156: <http://trac.webkit.org/changeset/166156> All reviewed patches have been landed. Closing bug. |