XZise has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/367907 )
Change subject: [FEAT] Rewrite commit message validator
......................................................................
[FEAT] Rewrite commit message validator
It converts the module wide functions into a class which acts as an iterator
which can internally store information. This allows each line check to
actually know "outside information" without the need of module wide variables,
like whether it is inside the footer or whether a change id already occurred.
This also converts all of the checks into yields so that it is almost nowhere
necessary to actually provide the line number it acts on. This changes a bit
the logic but also makes sure that the line numbers are now sequential.
Change-Id: If77a0abc8c46334e6ebbf415140c12375a4870f8
---
M commit_message_validator/__init__.py
M commit_message_validator/tests/data/check_message_errors.out
2 files changed, 108 insertions(+), 119 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/integration/commit-message-validator
refs/changes/07/367907/1
diff --git a/commit_message_validator/__init__.py
b/commit_message_validator/__init__.py
index 848d61b..175b795 100644
--- a/commit_message_validator/__init__.py
+++ b/commit_message_validator/__init__.py
@@ -81,8 +81,10 @@
return RE_CHANGEID.match(s)
-def line_errors(lineno, line):
- """Check a commit message line to see if it has errors.
+class CheckMessage(object):
+
+ """
+ Iterator to check a commit message line for errors.
Checks:
- First line <=80 characters
@@ -94,62 +96,113 @@
- "Change-Id:" is followed one change id ("I...")
- No "Task: ", "Fixes: ", "Closes: " lines
"""
- # First line <=80
- if lineno == 0:
- if len(line) > 80:
- yield "First line should be <=80 characters"
- m = RE_SUBJECT_BUG_OR_TASK.match(line)
- if m:
- yield "Do not define bug in the header"
- # Second line blank
- elif lineno == 1:
- if line:
- yield "Second line should be empty"
+ def __init__(self, lines):
+ self._lines = lines
+ self._first_changeid = False
+ self._in_footers = False
- # No line >100 unless it is all a URL
- if len(line) > 100 and not RE_URL.match(line):
- yield "Line should be <=100 characters"
+ def check_line(self, lineno):
+ line = self._lines[lineno]
+ # First line <=80
+ if lineno == 0:
+ if len(line) > 80:
+ yield "First line should be <=80 characters"
+ m = RE_SUBJECT_BUG_OR_TASK.match(line)
+ if m:
+ yield "Do not define bug in the header"
- # Look for and validate footer lines
- m = RE_FOOTER.match(line)
- if m:
- name = m.group('name')
- normalized_name = name.lower()
- ws = m.group('ws')
- value = m.group('value')
+ # Second line blank
+ elif lineno == 1:
+ if line:
+ yield "Second line should be empty"
- if normalized_name not in FOOTERS:
- # Meh. Not a name we care about
+ # No line >100 unless it is all a URL
+ elif len(line) > 100 and not RE_URL.match(line):
+ yield "Line should be <=100 characters"
+
+ if not line:
+ if self._in_footers:
+ yield "Unexpected blank line"
return
- if normalized_name in BAD_FOOTERS:
- # Treat as the correct name for the rest of the rules
- normalized_name = BAD_FOOTERS[normalized_name]
+ # Look for and validate footer lines
+ m = RE_FOOTER.match(line)
+ if m:
+ name = m.group('name')
+ normalized_name = name.lower()
+ ws = m.group('ws')
+ value = m.group('value')
- if normalized_name == 'bug':
- if name != 'Bug':
- yield "Use 'Bug:' not '{0}:'".format(name)
- if not is_valid_bug_id(value):
- yield "Bug: value must be a single phabricator task ID"
+ if normalized_name in FOOTERS:
+ if lineno > 0 and not self._lines[lineno - 1]:
+ self._in_footers = True
+ elif not self._in_footers:
+ yield "Expected '{0}:' to be in footer".format(name)
+ else:
+ if self._in_footers:
+ yield "Unexpected line in footers"
+ else:
+ # Meh. Not a name we care about
+ return
- elif normalized_name == 'depends-on':
- if name != 'Depends-On':
- yield "Use 'Depends-On:' not '%s:'" % name
- if not is_valid_change_id(value):
- yield "Depends-On: value must be a single Gerrit change id"
+ if normalized_name in BAD_FOOTERS:
+ # Treat as the correct name for the rest of the rules
+ normalized_name = BAD_FOOTERS[normalized_name]
- elif normalized_name == 'change-id':
- if name != 'Change-Id':
- yield "Use 'Change-Id:' not '%s:'" % name
- if not is_valid_change_id(value):
- yield "Change-Id: value must be a single Gerrit change id"
+ if normalized_name == 'bug':
+ if name != 'Bug':
+ yield "Use 'Bug:' not '{0}:'".format(name)
+ if not is_valid_bug_id(value):
+ yield "Bug: value must be a single phabricator task ID"
- elif name[0].upper() != name[0]:
- yield "'%s:' must start with a capital letter" % name
+ elif normalized_name == 'depends-on':
+ if name != 'Depends-On':
+ yield "Use 'Depends-On:' not '%s:'" % name
+ if not is_valid_change_id(value):
+ yield "Depends-On: value must be a single Gerrit change id"
- if ws != ' ':
- yield "Expected one space after '%s:'" % name
+ elif normalized_name == 'change-id':
+ if name != 'Change-Id':
+ yield "Use 'Change-Id:' not '%s:'" % name
+ if not is_valid_change_id(value):
+ yield "Change-Id: value must be a single Gerrit change id"
+ if self._first_changeid is not False:
+ yield ("Extra Change-Id found, first at "
+ "{0}".format(self._first_changeid))
+ else:
+ self._first_changeid = lineno + 1
+
+ elif name[0].upper() != name[0]:
+ yield "'%s:' must start with a capital letter" % name
+
+ if (normalized_name in BEFORE_CHANGE_ID and
+ self._first_changeid is not False):
+ yield ("Expected '{0}:' to come before Change-Id on line "
+ "{1}").format(name, self._first_changeid)
+
+ if ws != ' ':
+ yield "Expected one space after '%s:'" % name
+
+ elif self._in_footers:
+ cherry_pick = RE_CHERRYPICK.match(line)
+ if cherry_pick:
+ if lineno < len(self._lines) - 1:
+ yield "Cherry pick line is not the last line"
+ else:
+ yield "Expected footer line to follow format of 'Name: ...'"
+
+ def __iter__(self):
+ for lineno in range(len(self._lines)):
+ for e in self.check_line(lineno):
+ yield 'Line {0}: {1}'.format(lineno + 1, e)
+
+ if len(self._lines) < 3:
+ yield ("Line {0}: Expected at least 3 "
+ "lines".format(len(self._lines)))
+
+ if self._first_changeid is False:
+ yield "Line {0}: Expected Change-Id".format(len(self._lines))
def check_message(lines):
@@ -164,72 +217,8 @@
- Any "Bug:" and "Depends-On:" lines come before "Change-Id:"
- "(cherry picked from commit ...)" is last line in footer if present
"""
- errors = []
- last_lineno = 0
- last_line = ''
- changeid_line = False
- cherrypick_line = False
- in_footer = False
-
- for lineno, line in enumerate(lines):
- rline = lineno + 1
- errors.extend('Line {0}: {1}'.format(rline, e)
- for e in line_errors(lineno, line))
-
- if in_footer:
- if line == '':
- errors.append(
- "Line {0}: Unexpected blank line".format(rline))
- elif not (RE_FOOTER.match(line) or RE_CHERRYPICK.match(line)):
- errors.append((
- "Line {0}: Expected footer line to follow format of "
- "'Name: ...'").format(rline))
-
- m = RE_FOOTER.match(line)
- if m:
- name = m.group('name')
- normalized_name = name.lower()
-
- if last_line == '' and normalized_name in FOOTERS:
- # The first footer after a blank line starts the footer
- in_footer = True
-
- if normalized_name in FOOTERS and not in_footer:
- errors.append(
- "Line {0}: Expected '{1}:' to be in footer".format(
- rline, name))
-
- if normalized_name == 'change-id':
- # Only expect one "Change-Id: " line
- if changeid_line is not False:
- errors.append(
- "Line {0}: Extra Change-Id found, first at {1}".format(
- rline, changeid_line))
- changeid_line = rline
-
- elif normalized_name in BEFORE_CHANGE_ID:
- if changeid_line is not False:
- errors.append((
- "Line {0}: Expected '{1}:' to come before "
- "Change-Id on line {2}").format(
- rline, name, changeid_line))
-
- elif RE_CHERRYPICK.match(line):
- cherrypick_line = rline
-
- last_lineno = rline
- last_line = line
-
- if last_lineno < 2:
- errors.append("Line %d: Expected at least 3 lines" % last_lineno)
-
- if changeid_line is False:
- errors.append("Line %d: Expected Change-Id" % last_lineno)
-
- if cherrypick_line and cherrypick_line != last_lineno:
- errors.append(
- "Line {0}: Cherry pick expected to be last line".format(
- cherrypick_line))
+ checker = CheckMessage(lines)
+ errors = list(checker)
print('commit-message-validator v%s' % __version__)
if errors:
diff --git a/commit_message_validator/tests/data/check_message_errors.out
b/commit_message_validator/tests/data/check_message_errors.out
index 9cea8be..4609a24 100644
--- a/commit_message_validator/tests/data/check_message_errors.out
+++ b/commit_message_validator/tests/data/check_message_errors.out
@@ -3,29 +3,29 @@
Line 1: First line should be <=80 characters
Line 2: Second line should be empty
Line 3: Line should be <=100 characters
+Line 4: Expected 'Fixes:' to be in footer
Line 4: Use 'Bug:' not 'Fixes:'
Line 4: Bug: value must be a single phabricator task ID
Line 4: Expected one space after 'Fixes:'
-Line 4: Expected 'Fixes:' to be in footer
+Line 5: Expected 'Closes:' to be in footer
Line 5: Use 'Bug:' not 'Closes:'
Line 5: Bug: value must be a single phabricator task ID
Line 5: Expected one space after 'Closes:'
-Line 5: Expected 'Closes:' to be in footer
-Line 6: Use 'Bug:' not 'Task:'
Line 6: Expected 'Task:' to be in footer
+Line 6: Use 'Bug:' not 'Task:'
+Line 7: Expected 'BUG:' to be in footer
Line 7: Use 'Bug:' not 'BUG:'
Line 7: Bug: value must be a single phabricator task ID
Line 7: Expected one space after 'BUG:'
-Line 7: Expected 'BUG:' to be in footer
-Line 8: Bug: value must be a single phabricator task ID
Line 8: Expected 'Bug:' to be in footer
+Line 8: Bug: value must be a single phabricator task ID
Line 12: Bug: value must be a single phabricator task ID
Line 13: Unexpected blank line
Line 15: Unexpected blank line
Line 16: Bug: value must be a single phabricator task ID
Line 19: Extra Change-Id found, first at 18
Line 20: Depends-On: value must be a single Gerrit change id
-Line 20: Expected 'Depends-On:' to come before Change-Id on line 19
+Line 20: Expected 'Depends-On:' to come before Change-Id on line 18
Line 21: Unexpected blank line
Line 22: Expected footer line to follow format of 'Name: ...'
Please review <https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines>
--
To view, visit https://gerrit.wikimedia.org/r/367907
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: If77a0abc8c46334e6ebbf415140c12375a4870f8
Gerrit-PatchSet: 1
Gerrit-Project: integration/commit-message-validator
Gerrit-Branch: master
Gerrit-Owner: XZise <commodorefabia...@gmx.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
