Changeset 56748 in webkit

Timestamp:

Mar 29, 2010 4:33:49 PM (14 years ago)

Author:

eric@webkit.org

Message:

2010-03-29 Eric Seidel <​eric@webkit.org>

Reviewed by Adam Barth.

ValidateReviewer step is draconian and un-tested
​https://bugs.webkit.org/show_bug.cgi?id=36792

ValidateReviewer logic was commented out in
​http://trac.webkit.org/changeset/56744
That was a symptom of the fact that validatereviewer.py
is too inflexible to be used when real humans are driving webkit-patch.
For now we just disable ValidateReviewer when humans are at the keyboard.

• Scripts/webkitpy/tool/steps/validatereviewer.py:
  • Only run when in non-interactive mode.
• Scripts/webkitpy/tool/steps/validatereviewer_unittest.py: Added.
  • Test our validation logic to make sure it's sane.

Location:

trunk/WebKitTools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/tool/steps/validatereviewer.py (modified) (2 diffs)
• Scripts/webkitpy/tool/steps/validatereviewer_unittest.py (added)

trunk/WebKitTools/ChangeLog

@@ +1 @@
+2010-03-29  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        ValidateReviewer step is draconian and un-tested
+        https://bugs.webkit.org/show_bug.cgi?id=36792
+
+        ValidateReviewer logic was commented out in
+        http://trac.webkit.org/changeset/56744
+        That was a symptom of the fact that validatereviewer.py
+        is too inflexible to be used when real humans are driving webkit-patch.
+        For now we just disable ValidateReviewer when humans are at the keyboard.
+
+        * Scripts/webkitpy/tool/steps/validatereviewer.py:
+         - Only run when in non-interactive mode.
+        * Scripts/webkitpy/tool/steps/validatereviewer_unittest.py: Added.
+         - Test our validation logic to make sure it's sane.
+
 2010-03-29  Chris Jerdonek  <cjerdonek@webkit.org>
 

trunk/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py

@@ -35 +35 @@
 
 
+# FIXME: Some of this logic should probably be unified with CommitterValidator?
 class ValidateReviewer(AbstractStep):
+    # FIXME: This should probably move onto ChangeLogEntry
+    def _has_valid_reviewer(self, changelog_entry):
+        if changelog_entry.reviewer():
+            return True
+        if re.search("unreviewed", changelog_entry.contents(), re.IGNORECASE):
+            return True
+        if re.search("rubber[ -]stamp", changelog_entry.contents(), re.IGNORECASE):
+            return True
+        return False
+
     def run(self, state):
+        # FIXME: For now we disable this check when a user is driving the script
+        # this check is too draconian (and too poorly tested) to foist upon users.
+        if not self._options.non_interactive:
+            return
         # FIXME: We should figure out how to handle the current working
         #        directory issue more globally.
@@ -42 +57 @@
         for changelog_path in self._tool.checkout().modified_changelogs():
             changelog_entry = ChangeLog(changelog_path).latest_entry()
-            if changelog_entry.reviewer():
-                continue
-            if re.search("unreviewed", changelog_entry.contents(), re.IGNORECASE):
-                continue
-            if re.search("rubber[ -]stamp", changelog_entry.contents(), re.IGNORECASE):
+            if self._has_valid_reviewer(changelog_entry):
                 continue
             reviewer_text = changelog_entry.reviewer_text()
-#            if reviewer_text:
-#                log("%s does not appear to be a valid reviewer according to committers.py.")
-#            error('%s neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).' % changelog_path)
+            if reviewer_text:
+                log("%s found in %s does not appear to be a valid reviewer according to committers.py." % (reviewer_text, changelog_path))
+            error('%s neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).' % changelog_path)