Fixes and suggestions

Author: gvwilson

tools/check

@@ -9,43 +9,69 @@
 #     $ tools/check
 
 import os
-import os.path
 import re
-
 import yaml
 
-def report_error(file_path, line_number, line, error_message):
-    print("Error at line {} of {}:\n\t{}\n{}".format(line_number,
-        file_path, line, error_message))
+#----------------------------------------
+# Error reporting.
 
-def report_missing(file_path, missing_element):
-    print("Error on {}: missing {}".format(file_path, missing_element))
+def report_error(file_path, line_number, line, error_message):
+    '''
+    FIXME: docstring.
+    '''
+    ERR_MSG = "Error at line {} of {}:\n\t{}\n{}"
+    print(ERR_MSG.format(line_number, file_path, line, error_message))
+
+def report_missing(present, file_path, missing_element):
+    '''
+    FIXME: docstring.
+    '''
+    ERR_MSG = "Error on {}: missing {}"
+    if not present:
+        print(ERR_MSG.format(file_path, missing_element))
 
 def report_missing_metadata(missing_element):
-    print("Error on YAML header: missing {}".format(missing_element))
+    '''
+    FIXME: docstring.
+    '''
+    ERR_MSG = "Error on YAML header: missing {}"
+    print(ERR_MSG.format(missing_element))
 
 def report_broken_link(file_path, line_number, link):
-    print("Broken link at line {} of {}:\n\tCan't find {}.".format(line_number,
-        file_path, link))
+    '''
+    FIXME: docstring.
+    '''
+    ERR_MSG = "Broken link at line {} of {}:\n\tCan't find {}."
+    print(ERR_MSG.format(line_number, file_path, link))
 
-def check_yaml(metadata, skip=[]):
+#----------------------------------------
+# Checking.
+
+def check_yaml(metadata):
     """
     Check if all metadata are present at YAML header.
-
-    :param skip: list of keys to skip check
     """
-    metadata_required = ["layout", "title", "minutes"]
-    for key in metadata_required:
-        if key not in skip and key not in metadata:
-            report_missing_metadata(key)
+    METADATA_REQUIRED = {"layout", "title", "minutes"}
+    for key in METADATA_REQUIRED - set(metadata.keys()):
+        report_missing_metadata(key)
 
 def check_lesson(file_path):
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     pass
 
 def check_discussion(file_path):
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     pass
 
 def check_index(file_path):
+    '''
+    FIXME: docstring.
+    And break this up into pieces -- it's too long.
+    '''
     # State variables
     in_yaml = False
     yaml_metadata = []
@@ -56,66 +82,82 @@ def check_index(file_path):
     # Load file and process it
     with open(file_path, 'r') as lines:
         for line_number, line in enumerate(lines):
-            if re.match('---', line):
+            if re.match('---', line): # what if there are multiple YAML blocks??
                 in_yaml = not in_yaml
             elif in_yaml:
                 yaml_metadata.append(line)
-            elif re.match('> ## Prerequisites', line):
+            elif re.match('> ## Prerequisites', line): # check this in the Markdown or in the generated HTML?
                 has_prerequisites = True
-            elif re.match('## Topics', line):
+            elif re.match('## Topics', line): # as above?
                 has_topics = True
-            elif re.match('## Other Resources', line):
+            elif re.match('## Other Resources', line): # as above
                 has_other_resources = True
             else:
+                ## Push this check into another function - this one is getting too long.
                 # Check if local links are valid
                 matches = re.search('\[.*\]\((?P<link>.*)\)', line)
                 if matches and not matches.group("link").startswith("http"):
-                    link = os.path.join(os.path.dirname(file_path),
-                            matches.group("link"))
+                    link = os.path.join(os.path.dirname(file_path), matches.group("link"))
                     if link.endswith(".html"):
-                        link = link.replace("html", "md")
+                        link = link.replace("html", "md") # NO: what about '03-html-editing.html' ?
                     if not os.path.exists(link):
                         report_broken_link(file_path, line_number, link)
 
+    ## Again, this function is too long - break it into sub-functions.
     # Check YAML
     yaml_metadata = yaml.load('\n'.join(yaml_metadata))
-    check_yaml(yaml_metadata, ["minutes"])
+    check_yaml(yaml_metadata, {"minutes"})
 
     # Check sections
-    if not has_prerequisites:
-        report_missing(file_path, "Prerequisites")
-    if not has_topics:
-        report_missing(file_path, "Topics")
-    if not has_other_resources:
-        report_missing(file_path, "Other Resources")
+    ## Note the refactoring: replaces three conditionals with one.
+    report_missing(has_prerequisites, file_path, "Prerequisites")
+    report_missing(has_topics, file_path, "Topics")
+    report_missing(has_other_resources, file_path, "Other Resources")
 
 def check_intructors(file_path):
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     pass
 
 def check_motivation(file_path):
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     pass
 
 def check_reference(file_path):
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     pass
 
 def check_file(file_path):
-    if re.search('[0-9]{2}-.*', file_path):
-        check_lesson(file_path)
-    elif re.search('discussion', file_path):
-        check_discussion(file_path)
-    elif re.search('index', file_path):
-        check_index(file_path)
-    elif re.search('instructors', file_path):
-        check_intructors(file_path)
-    elif re.search("motivation", file_path):
-        check_motivation(file_path)
-    elif re.search("reference", file_path):
-        check_reference(file_path)
-
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
+    ## Functions are objects, and so can be put in tables like the one below.
+    CONTROL = (
+        ('[0-9]{2}-.*', check_lesson),
+        ('discussion',  check_discussion),
+        ('index',       check_index),
+        ('instructors', check_intructors),
+        ("motivation",  check_motivation),
+        ("reference",   check_reference)
+    )
+    for (pattern, checker) in CONTROL:
+        if re.search(pattern, file_path):
+            checker(file_path)
+
+## main doesn't take sys.argv[1:] or the like?  Will help with testing...
 def main():
+    '''
+    FIXME: docstring telling people what you want them to write here.
+    '''
     lessons_file = os.listdir("pages")
     for lesson in lessons_file:
         if lesson.endswith('.md'):
+            ## Why not os.path.join('pages', lesson) ?
             check_file('pages/{}'.format(lesson))
 
 if __name__ == "__main__":