From d5240ec4c718773ce0cc23ecdad87b4332bb9dd8 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 20:59:05 +0100 Subject: [PATCH] check-files.py: clean up class structure Line issue trackers are conceptually a subclass of file issue trackers: they're file issue trackers where issues arise from checking each line independently. So make it an actual subclass. Pylint pointed out the design smell: there was an abstract method that wasn't always overridden in concrete child classes. --- tests/scripts/check-files.py | 71 ++++++++++++++++++++---------------- 1 file changed, 39 insertions(+), 32 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 92cae1dc2..a6743bbfc 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -19,10 +19,12 @@ import codecs import sys -class IssueTracker(object): - """Base class for issue tracking. Issues should inherit from this and - 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.""" +class FileIssueTracker(object): + """Base class for file-wide issue tracking. + + To implement a checker that processes a file as a whole, inherit from + this class and implement `check_file_for_issue`. + """ def __init__(self): self.heading = "" @@ -35,23 +37,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: - for i, line in enumerate(iter(f.readline, b"")): - self.check_file_line(filepath, line, i + 1) + raise NotImplementedError 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,8 +57,26 @@ 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 issue_with_line(self, line, filepath): + raise NotImplementedError + + 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): + with open(filepath, "rb") as f: + 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.""" @@ -80,7 +91,7 @@ class PermissionIssueTracker(IssueTracker): 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).""" @@ -94,7 +105,7 @@ 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.""" @@ -108,18 +119,18 @@ 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): 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): @@ -127,11 +138,11 @@ class TrailingWhitespaceIssueTracker(IssueTracker): 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): @@ -141,11 +152,11 @@ class TabIssueTracker(IssueTracker): "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.""" @@ -153,22 +164,18 @@ class MergeArtifactIssueTracker(IssueTracker): 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): - if self.issue_with_line(filepath, line): - self.record_issue(filepath, line_number) - -class TodoIssueTracker(IssueTracker): +class TodoIssueTracker(LineIssueTracker): """Track lines containing ``TODO``.""" def __init__(self): @@ -180,7 +187,7 @@ class TodoIssueTracker(IssueTracker): "pull_request_template.md", ] - def issue_with_line(self, line): + def issue_with_line(self, line, _filepath): return b"todo" in line.lower()