From 4fb6678da52a838a7279a52fb216d51fcbba25c0 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 20:35:31 +0100 Subject: [PATCH 1/4] check-files.py: document some classes and methods Document all classes and longer methods. Declare a static method as such. Pointed out by pylint. --- tests/scripts/check-files.py | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 7ea321f88..c11188c0b 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -66,6 +66,9 @@ class IssueTracker(object): class PermissionIssueTracker(IssueTracker): + """Track files with bad permissions. + + Files that are not executable scripts must not be executable.""" def __init__(self): super().__init__() @@ -78,6 +81,8 @@ class PermissionIssueTracker(IssueTracker): class EndOfFileNewlineIssueTracker(IssueTracker): + """Track files that end with an incomplete line + (no newline character at the end of the last line).""" def __init__(self): super().__init__() @@ -90,6 +95,8 @@ class EndOfFileNewlineIssueTracker(IssueTracker): class Utf8BomIssueTracker(IssueTracker): + """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): super().__init__() @@ -102,6 +109,7 @@ class Utf8BomIssueTracker(IssueTracker): class LineEndingIssueTracker(IssueTracker): + """Track files with non-Unix line endings (i.e. files with CR).""" def __init__(self): super().__init__() @@ -112,6 +120,7 @@ class LineEndingIssueTracker(IssueTracker): class TrailingWhitespaceIssueTracker(IssueTracker): + """Track lines with trailing whitespace.""" def __init__(self): super().__init__() @@ -123,6 +132,7 @@ class TrailingWhitespaceIssueTracker(IssueTracker): class TabIssueTracker(IssueTracker): + """Track lines with tabs.""" def __init__(self): super().__init__() @@ -136,6 +146,8 @@ class TabIssueTracker(IssueTracker): class MergeArtifactIssueTracker(IssueTracker): + """Track lines with merge artifacts. + These are leftovers from a ``git merge`` that wasn't fully edited.""" def __init__(self): super().__init__() @@ -157,6 +169,7 @@ class MergeArtifactIssueTracker(IssueTracker): self.record_issue(filepath, line_number) class TodoIssueTracker(IssueTracker): + """Track lines containing ``TODO``.""" def __init__(self): super().__init__() @@ -170,8 +183,12 @@ class TodoIssueTracker(IssueTracker): 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 +213,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") From 7194ecb3fb4503c87db0159aec81d93fa14742fd Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 20:59:05 +0100 Subject: [PATCH 2/4] 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 c11188c0b..38a6ba539 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): @@ -178,7 +185,7 @@ class TodoIssueTracker(IssueTracker): __file__, "benchmark.c", "pull_request_template.md" ] - def issue_with_line(self, line): + def issue_with_line(self, line, _filepath): return b"todo" in line.lower() From fb8c373a150c49df1f39098d10d2e0e49b4a6c2d Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 21:10:04 +0100 Subject: [PATCH 3/4] check-files.py: use class fields for class-wide constants In an issue tracker, heading and files_exemptions are class-wide constants, so make them so instead of being per-instance fields. --- tests/scripts/check-files.py | 62 ++++++++++++++++-------------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index 38a6ba539..bc5b8f5bb 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -23,12 +23,19 @@ 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`. + 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): @@ -81,9 +88,7 @@ class PermissionIssueTracker(FileIssueTracker): Files that are not executable scripts must not be executable.""" - def __init__(self): - super().__init__() - self.heading = "Incorrect permissions:" + heading = "Incorrect permissions:" def check_file_for_issue(self, filepath): if not (os.access(filepath, os.X_OK) == @@ -95,9 +100,7 @@ class EndOfFileNewlineIssueTracker(FileIssueTracker): """Track files that end with an incomplete line (no newline character at the end of the last line).""" - def __init__(self): - super().__init__() - self.heading = "Missing newline at end of file:" + heading = "Missing newline at end of file:" def check_file_for_issue(self, filepath): with open(filepath, "rb") as f: @@ -109,9 +112,7 @@ 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): - super().__init__() - self.heading = "UTF-8 BOM present:" + heading = "UTF-8 BOM present:" def check_file_for_issue(self, filepath): with open(filepath, "rb") as f: @@ -122,9 +123,7 @@ class Utf8BomIssueTracker(FileIssueTracker): 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:" + heading = "Non Unix line endings:" def issue_with_line(self, line, _filepath): return b"\r" in line @@ -133,10 +132,8 @@ class LineEndingIssueTracker(LineIssueTracker): class TrailingWhitespaceIssueTracker(LineIssueTracker): """Track lines with trailing whitespace.""" - def __init__(self): - super().__init__() - self.heading = "Trailing whitespace:" - self.files_exemptions = [".md"] + heading = "Trailing whitespace:" + files_exemptions = frozenset(".md") def issue_with_line(self, line, _filepath): return line.rstrip(b"\r\n") != line.rstrip() @@ -145,12 +142,11 @@ class TrailingWhitespaceIssueTracker(LineIssueTracker): class TabIssueTracker(LineIssueTracker): """Track lines with tabs.""" - def __init__(self): - super().__init__() - self.heading = "Tabs present:" - self.files_exemptions = [ - "Makefile", "generate_visualc_files.pl" - ] + heading = "Tabs present:" + files_exemptions = frozenset([ + "Makefile", + "generate_visualc_files.pl", + ]) def issue_with_line(self, line, _filepath): return b"\t" in line @@ -160,9 +156,7 @@ class MergeArtifactIssueTracker(LineIssueTracker): """Track lines with merge artifacts. These are leftovers from a ``git merge`` that wasn't fully edited.""" - def __init__(self): - super().__init__() - self.heading = "Merge artifact:" + heading = "Merge artifact:" def issue_with_line(self, line, _filepath): # Detect leftover git conflict markers. @@ -178,12 +172,12 @@ class MergeArtifactIssueTracker(LineIssueTracker): class TodoIssueTracker(LineIssueTracker): """Track lines containing ``TODO``.""" - def __init__(self): - super().__init__() - self.heading = "TODO present:" - self.files_exemptions = [ - __file__, "benchmark.c", "pull_request_template.md" - ] + heading = "TODO present:" + files_exemptions = frozenset([ + os.path.basename(__file__), + "benchmark.c", + "pull_request_template.md", + ]) def issue_with_line(self, line, _filepath): return b"todo" in line.lower() From de12823a1895ab87f3bd8865f2a3ef1cd8c79d09 Mon Sep 17 00:00:00 2001 From: Gilles Peskine Date: Mon, 25 Feb 2019 21:24:27 +0100 Subject: [PATCH 4/4] check-files.py: readability improvement in permission check --- tests/scripts/check-files.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/scripts/check-files.py b/tests/scripts/check-files.py index bc5b8f5bb..0bf01206e 100755 --- a/tests/scripts/check-files.py +++ b/tests/scripts/check-files.py @@ -91,8 +91,9 @@ class PermissionIssueTracker(FileIssueTracker): heading = "Incorrect permissions:" def check_file_for_issue(self, filepath): - if not (os.access(filepath, os.X_OK) == - filepath.endswith((".sh", ".pl", ".py"))): + 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