Updating comments and error messages for better readability and validation (#1457)

tests/definitions_test.py

@@ -49,6 +49,9 @@ def _get_image_files():
     return file_list
 
 def _decimal_file_handler(uri):
+    """
+    Handler to work with floating decimals that fail normal validation.
+    """
     with urlopen(uri) as url:
         result = json.loads(url.read().decode("utf-8"), parse_float=decimal.Decimal)
     return result
@@ -68,14 +71,14 @@ def test_definitions(file_path, schema):
     """
     Validate each definition file using the provided JSON schema and check for duplicate entries.
     """
-    # Check file extension
+    # Check file extension. Only .yml or .yaml files are supported.
     assert file_path.split('.')[-1] in ('yaml', 'yml'), f"Invalid file extension: {file_path}"
 
     # Read file
     with open(file_path) as definition_file:
         content = definition_file.read()
 
-    # Check for trailing newline
+    # Check for trailing newline. YAML files must end with an emtpy newline.
     assert content.endswith('\n'), "Missing trailing newline"
 
     # Load YAML data from file
@@ -88,23 +91,28 @@ def test_definitions(file_path, schema):
             schema,
             handlers={"file": _decimal_file_handler},
         )
+        # Validate definition against schema
         Draft4Validator(schema, resolver=resolver).validate(definition)
     except ValidationError as e:
+        # Schema validation failure. Ensure you are following the proper format.
         pytest.fail(f"{file_path} failed validation: {e}", False)
 
     # Identify if the definition is for a Device or Module
     if "device-types" in file_path:
+        # A device
         this_device = DeviceType(definition, file_path)
     else:
+        # A module
         this_device = ModuleType(definition, file_path)
 
-    # Verify the slug is valid if the definition is a Device
+    # Verify the slug is valid, only if the definition type is a Device
     if this_device.isDevice:
         assert this_device.verify_slug(), pytest.fail(this_device.failureMessage, False)
 
+    # Verify the filename is valid. Must either be the model or part_number.
     assert verify_filename(this_device), pytest.fail(this_device.failureMessage, False)
 
-    # Check for duplicate components
+    # Check for duplicate components within the definition
     for component_type in COMPONENT_TYPES:
         known_names = set()
         defined_components = definition.get(component_type, [])
@@ -114,7 +122,7 @@ def test_definitions(file_path, schema):
                 pytest.fail(f'Duplicate entry "{name}" in {component_type} list', False)
             known_names.add(name)
 
-    # Check for empty quotes
+    # Check for empty quotes and fail if found
     def iterdict(var):
         for dict_value in var.values():
             if isinstance(dict_value, dict):
@@ -141,12 +149,14 @@ def test_definitions(file_path, schema):
         elif len(manufacturer_images)>2:
             pytest.fail(f'More than 2 images found for device with slug {this_device.get_slug()}: {manufacturer_images}', False)
 
+        # If front_image is True, verify that a front image exists
         if(definition.get('front_image')):
             front_image = [image_path.split('/')[2] for image_path in manufacturer_images if os.path.basename(image_path).split('.')[1] == 'front']
 
             if not front_image:
                 pytest.fail(f'{file_path} has front_image set to True but no matching image found for device ({manufacturer_images})', False)
 
+        # If rear_image is True, verify that a front image exists
         if(definition.get('rear_image')):
             rear_image = [image_path.split('/')[2] for image_path in manufacturer_images if os.path.basename(image_path).split('.')[1] == 'rear']
 

tests/device_types.py

@@ -17,9 +17,6 @@ class DeviceType:
         self._slug_part_number = self._slugify_part_number()
         self.failureMessage = None
 
-    def get_manufacturer(self):
-        return self.manufacturer
-
     def _slugify_manufacturer(self):
         return self.manufacturer.casefold().replace(" ", "-").replace("sfp+", "sfpp").replace("poe+", "poep").replace("-+", "-plus-").replace("+", "-plus").replace("_", "-").replace("!", "").replace("/", "-").replace(",", "").replace("'", "").replace("*", "-").replace("&", "and")
 
@@ -28,18 +25,12 @@ class DeviceType:
             return self.slug
         return None
 
-    def get_model(self):
-        return self.model
-
     def _slugify_model(self):
         slugified = self.model.casefold().replace(" ", "-").replace("sfp+", "sfpp").replace("poe+", "poep").replace("-+", "-plus").replace("+", "-plus-").replace("_", "-").replace("&", "-and-").replace("!", "").replace("/", "-").replace(",", "").replace("'", "").replace("*", "-")
         if slugified.endswith("-"):
             slugified = slugified[:-1]
         return slugified
 
-    def get_part_number(self):
-        return self.part_number
-
     def _slugify_part_number(self):
         slugified = self.part_number.casefold().replace(" ", "-").replace("-+", "-plus").replace("+", "-plus-").replace("_", "-").replace("&", "-and-").replace("!", "").replace("/", "-").replace(",", "").replace("'", "").replace("*", "-")
         if slugified.endswith("-"):
@@ -52,17 +43,17 @@ class DeviceType:
     def verify_slug(self):
         # Verify the slug is unique, and not already known
         if self.slug in KNOWN_SLUGS:
-            self.failureMessage = f'{self.file_path} device type has duplicate slug "{self.slug}"'
+            self.failureMessage = f'{self.file_path} has a duplicate slug "{self.slug}"'
             return False
 
         # Verify the manufacturer is appended to the slug
         if not self.slug.startswith(self._slug_manufacturer):
-            self.failureMessage = f'{self.file_path} device type has slug "{self.slug}" which does not start with manufacturer "{self.manufacturer}"'
+            self.failureMessage = f'{self.file_path} contains slug "{self.slug}". Does not start with manufacturer: "{self.manufacturer.casefold()}-"'
             return False
 
         # Verify the slug ends with either the model or part number
         if not (self.slug.endswith(self._slug_model) or self.slug.endswith(self._slug_part_number)):
-            self.failureMessage = f'{self.file_path} has slug "{self.slug}". Does not end with the model "{self._slug_model}" or part number "{self._slug_part_number}"'
+            self.failureMessage = f'{self.file_path} has slug "{self.slug}". Does not end with the model "{self._slug_model}" or part_number "{self._slug_part_number}"'
             return False
 
         # Add the slug to the list of known slugs
@@ -82,9 +73,6 @@ class ModuleType:
         self.part_number = definition.get('part_number', "")
         self._slug_part_number = self._slugify_part_number()
 
-    def get_manufacturer(self):
-        return self.manufacturer
-
     def get_filepath(self):
         return self.file_path
 
@@ -105,7 +93,7 @@ def verify_filename(device: (DeviceType or ModuleType)):
     filename = tail.rsplit(".", 1)[0].casefold()
 
     if not (filename == device._slug_model or filename == device._slug_part_number or filename == device.part_number.casefold()):
-        device.failureMessage = f'{device.file_path} file is not either the model "{device._slug_model}" or part_number "{device.part_number} / {device._slug_part_number}"'
+        device.failureMessage = f'{device.file_path} file name is invalid. Must be either the model "{device._slug_model}" or part_number "{device.part_number} / {device._slug_part_number}"'
         return False
 
     return True
\ No newline at end of file