Merge remote-tracking branch 'origin/pr/2471' into mbedtls-2.7

* origin/pr/2471:
  check-files.py: readability improvement in permission check
  check-files.py: use class fields for class-wide constants
  check-files.py: clean up class structure
  check-files.py: document some classes and methods

tests/scripts/check-files.py

@@ -19,14 +19,23 @@ import codecs
 import sys
 
 
-class IssueTracker(object):
+class FileIssueTracker(object):
-    """Base class for issue tracking. Issues should inherit from this and
+    """Base class for file-wide issue tracking.
-    overwrite either issue_with_line if they check the file line by line, or
+
-    overwrite check_file_for_issue if they check the file as a whole."""
+    To implement a checker that processes a file as a whole, inherit from
+    this class and implement `check_file_for_issue` and define ``heading``.
+
+    ``files_exemptions``: files whose name ends with a string in this set
+     will not be checked.
+
+    ``heading``: human-readable description of the issue
+    """
+
+    files_exemptions = frozenset()
+    # heading must be defined in derived classes.
+    # pylint: disable=no-member
 
     def __init__(self):
-        self.heading = ""
-        self.files_exemptions = []
         self.files_with_issues = {}
 
     def should_check_file(self, filepath):
@@ -35,23 +44,14 @@ class IssueTracker(object):
                 return False
         return True
 
-    def issue_with_line(self, line):
-        raise NotImplementedError
-
     def check_file_for_issue(self, filepath):
-        with open(filepath, "rb") as f:
+        raise NotImplementedError
-            for i, line in enumerate(iter(f.readline, b"")):
-                self.check_file_line(filepath, line, i + 1)
 
     def record_issue(self, filepath, line_number):
         if filepath not in self.files_with_issues.keys():
             self.files_with_issues[filepath] = []
         self.files_with_issues[filepath].append(line_number)
 
-    def check_file_line(self, filepath, line, line_number):
-        if self.issue_with_line(line):
-            self.record_issue(filepath, line_number)
-
     def output_file_issues(self, logger):
         if self.files_with_issues.values():
             logger.info(self.heading)
@@ -64,24 +64,44 @@ class IssueTracker(object):
                     logger.info(filename)
             logger.info("")
 
+class LineIssueTracker(FileIssueTracker):
+    """Base class for line-by-line issue tracking.
 
-class PermissionIssueTracker(IssueTracker):
+    To implement a checker that processes files line by line, inherit from
+    this class and implement `line_with_issue`.
+    """
 
-    def __init__(self):
+    def issue_with_line(self, line, filepath):
-        super().__init__()
+        raise NotImplementedError
-        self.heading = "Incorrect permissions:"
+
+    def check_file_line(self, filepath, line, line_number):
+        if self.issue_with_line(line, filepath):
+            self.record_issue(filepath, line_number)
 
     def check_file_for_issue(self, filepath):
-        if not (os.access(filepath, os.X_OK) ==
+        with open(filepath, "rb") as f:
-                filepath.endswith((".sh", ".pl", ".py"))):
+            for i, line in enumerate(iter(f.readline, b"")):
+                self.check_file_line(filepath, line, i + 1)
+
+class PermissionIssueTracker(FileIssueTracker):
+    """Track files with bad permissions.
+
+    Files that are not executable scripts must not be executable."""
+
+    heading = "Incorrect permissions:"
+
+    def check_file_for_issue(self, filepath):
+        is_executable = os.access(filepath, os.X_OK)
+        should_be_executable = filepath.endswith((".sh", ".pl", ".py"))
+        if is_executable != should_be_executable:
             self.files_with_issues[filepath] = None
 
 
-class EndOfFileNewlineIssueTracker(IssueTracker):
+class EndOfFileNewlineIssueTracker(FileIssueTracker):
+    """Track files that end with an incomplete line
+    (no newline character at the end of the last line)."""
 
-    def __init__(self):
+    heading = "Missing newline at end of file:"
-        super().__init__()
-        self.heading = "Missing newline at end of file:"
 
     def check_file_for_issue(self, filepath):
         with open(filepath, "rb") as f:
@@ -89,11 +109,11 @@ class EndOfFileNewlineIssueTracker(IssueTracker):
                 self.files_with_issues[filepath] = None
 
 
-class Utf8BomIssueTracker(IssueTracker):
+class Utf8BomIssueTracker(FileIssueTracker):
+    """Track files that start with a UTF-8 BOM.
+    Files should be ASCII or UTF-8. Valid UTF-8 does not start with a BOM."""
 
-    def __init__(self):
+    heading = "UTF-8 BOM present:"
-        super().__init__()
-        self.heading = "UTF-8 BOM present:"
 
     def check_file_for_issue(self, filepath):
         with open(filepath, "rb") as f:
@@ -101,77 +121,76 @@ class Utf8BomIssueTracker(IssueTracker):
                 self.files_with_issues[filepath] = None
 
 
-class LineEndingIssueTracker(IssueTracker):
+class LineEndingIssueTracker(LineIssueTracker):
+    """Track files with non-Unix line endings (i.e. files with CR)."""
 
-    def __init__(self):
+    heading = "Non Unix line endings:"
-        super().__init__()
-        self.heading = "Non Unix line endings:"
 
-    def issue_with_line(self, line):
+    def issue_with_line(self, line, _filepath):
         return b"\r" in line
 
 
-class TrailingWhitespaceIssueTracker(IssueTracker):
+class TrailingWhitespaceIssueTracker(LineIssueTracker):
+    """Track lines with trailing whitespace."""
 
-    def __init__(self):
+    heading = "Trailing whitespace:"
-        super().__init__()
+    files_exemptions = frozenset(".md")
-        self.heading = "Trailing whitespace:"
-        self.files_exemptions = [".md"]
 
-    def issue_with_line(self, line):
+    def issue_with_line(self, line, _filepath):
         return line.rstrip(b"\r\n") != line.rstrip()
 
 
-class TabIssueTracker(IssueTracker):
+class TabIssueTracker(LineIssueTracker):
+    """Track lines with tabs."""
 
-    def __init__(self):
+    heading = "Tabs present:"
-        super().__init__()
+    files_exemptions = frozenset([
-        self.heading = "Tabs present:"
+        "Makefile",
-        self.files_exemptions = [
+        "generate_visualc_files.pl",
-            "Makefile", "generate_visualc_files.pl"
+    ])
-        ]
 
-    def issue_with_line(self, line):
+    def issue_with_line(self, line, _filepath):
         return b"\t" in line
 
 
-class MergeArtifactIssueTracker(IssueTracker):
+class MergeArtifactIssueTracker(LineIssueTracker):
+    """Track lines with merge artifacts.
+    These are leftovers from a ``git merge`` that wasn't fully edited."""
 
-    def __init__(self):
+    heading = "Merge artifact:"
-        super().__init__()
-        self.heading = "Merge artifact:"
 
-    def issue_with_line(self, filepath, line):
+    def issue_with_line(self, line, _filepath):
         # Detect leftover git conflict markers.
         if line.startswith(b'<<<<<<< ') or line.startswith(b'>>>>>>> '):
             return True
         if line.startswith(b'||||||| '): # from merge.conflictStyle=diff3
             return True
         if line.rstrip(b'\r\n') == b'=======' and \
-           not filepath.endswith('.md'):
+           not _filepath.endswith('.md'):
             return True
         return False
 
-    def check_file_line(self, filepath, line, line_number):
+class TodoIssueTracker(LineIssueTracker):
-        if self.issue_with_line(filepath, line):
+    """Track lines containing ``TODO``."""
-            self.record_issue(filepath, line_number)
 
-class TodoIssueTracker(IssueTracker):
+    heading = "TODO present:"
+    files_exemptions = frozenset([
+        os.path.basename(__file__),
+        "benchmark.c",
+        "pull_request_template.md",
+    ])
 
-    def __init__(self):
+    def issue_with_line(self, line, _filepath):
-        super().__init__()
-        self.heading = "TODO present:"
-        self.files_exemptions = [
-            __file__, "benchmark.c", "pull_request_template.md"
-        ]
-
-    def issue_with_line(self, line):
         return b"todo" in line.lower()
 
 
 class IntegrityChecker(object):
+    """Sanity-check files under the current directory."""
 
     def __init__(self, log_file):
+        """Instantiate the sanity checker.
+        Check files under the current directory.
+        Write a report of issues to log_file."""
         self.check_repo_path()
         self.logger = None
         self.setup_logger(log_file)
@@ -196,7 +215,8 @@ class IntegrityChecker(object):
             TodoIssueTracker(),
         ]
 
-    def check_repo_path(self):
+    @staticmethod
+    def check_repo_path():
         if not all(os.path.isdir(d) for d in ["include", "library", "tests"]):
             raise Exception("Must be run from Mbed TLS root")