[Webkit-unassigned] [Bug 122654] webkitbot rollout ChangeLogs should be nicer

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=122654


Csaba Osztrogonác <ossy at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #226314|review?                     |review-, commit-queue-
               Flag|                            |




--- Comment #3 from Csaba Osztrogonác <ossy at webkit.org>  2014-03-10 11:08:05 PST ---
(From update of attachment 226314)
View in context: https://bugs.webkit.org/attachment.cgi?id=226314&action=review

Great work, I really like this new rollout changelog format.

The patch looks good to me in general, but I have some minor comments.

> Tools/ChangeLog:8
> +        Added bug urls and descriptions of rolled out patches to the rollout changelog if they are present in the original changelog. Additionally removed the list of changed files and functions.

This line is too long, wrap it please.

> Tools/ChangeLog:11
> +        (ChangeLog.update_with_unreviewed_message): Cut off the list of modified files

It is a sentence, please close it with "." (not only here)

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:48
> +            if index < len(description_list) and index < len(bug_url_list):

Do we really need this check? 

I think the length of revision_list, description_list and bug_url_list must be same always.
If they are same, we don't need this check. If they aren't same, it is a bug which should be fixed.

> Tools/Scripts/webkitpy/tool/steps/preparechangelogforrevert.py:64
>          bug_url = self._tool.bugs.bug_url_for_bug_id(state["bug_id"]) if state["bug_id"] else None
> -        message = self._message_for_revert(state["revision_list"], state["reason"], bug_url)
> +        for bug_id in state["bug_id_list"]:
> +            bug_url_list.append(self._tool.bugs.bug_url_for_bug_id(bug_id))
> +        message = self._message_for_revert(state["revision_list"], state["reason"], state["description_list"], bug_url_list)

You missed to pass bug_url to _message_for_revert. Additionally
these names are a little bit confusing: bug_url and bug_url_list

I managed to understand that bug_url is the URL of the rollout bug (if there is)
and bug_url_list contains the urls for rolled out patches. But it would be great
to have better names, for example rollout_bug_url instead of bug_url and
reverted_bug_url_list instead of bug_url_list.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.