diff --git a/hed/schema/schema_comparer.py b/hed/schema/schema_comparer.py index 242190f4..3ca5bd47 100644 --- a/hed/schema/schema_comparer.py +++ b/hed/schema/schema_comparer.py @@ -290,47 +290,61 @@ def pretty_print_change_dict(self, change_dict, title="Schema changes", use_mark Parameters: change_dict (dict): Dictionary of changes as returned by gather_schema_changes. Format: {section_key -> [{"change_type": str, "change": str, "tag": str}]} - title (str): Title for the change report. Default is "Schema changes". - use_markdown (bool): If True, use markdown formatting with bold headers (** **) and - bullet point prefixes (" - "). If False, use plain text with - tabs for indentation. Default is True. + title (str): Title for the change report. Default is "Schema changes". In markdown mode + it becomes a level-two heading (``## title``); an empty title emits no heading. + use_markdown (bool): If True, use the PRERELEASE_CHANGES.md layout: ``## title``, a blank + line, then per section a bold ``**Name:**`` header, a blank line, + ``- `` bullets, and a blank line before the next section. If False, + use plain text with a bare title, plain headers, and tab-indented + lines. Default is True. Returns: - str: Formatted string representation of the changes. Sections are sorted by order - in SECTION_ENTRY_NAMES, changes within each section sorted by severity - (Major → Minor → Patch → Unknown). Empty if change_dict is empty. + str: Formatted string representation of the changes, ending in a single newline. Sections + are in the order of SECTION_ENTRY_NAMES whatever the order of change_dict (keys not in + SECTION_ENTRY_NAMES follow, in dict order); changes within each section keep the order + given, which is by severity (Major -> Minor -> Patch -> Unknown) for gather_schema_changes + output. Empty if change_dict is empty. Example: >>> changes = comparer.gather_schema_changes() >>> output = comparer.pretty_print_change_dict( ... changes, - ... title="HED 8.3.0 → 8.4.0 Changes", + ... title="HED 8.3.0 -> 8.4.0 Changes", ... use_markdown=True ... ) >>> print(output) >>> # Can be written to file for changelog documentation """ + if not change_dict: + return "" final_strings = [] - line_prefix = " - " if use_markdown else "\t" - if change_dict: - final_strings.append(title) - final_strings.append("") # add blank line - for section_key, section_dict in change_dict.items(): - name = self.SECTION_ENTRY_NAMES_PLURAL.get(section_key, section_key) - line_endings = "**" if use_markdown else "" - final_strings.append(f"{line_endings}{name}:{line_endings}") - for item in section_dict: - change, tag, change_type = item["change"], item["tag"], item["change_type"] - final_strings.append(f"{line_prefix}{tag} ({change_type}): {change}") + line_prefix = "- " if use_markdown else "\t" + if title: + final_strings.append(f"## {title}" if use_markdown else title) + final_strings.append("") + known_keys = [key for key in self.SECTION_ENTRY_NAMES if key in change_dict] + extra_keys = [key for key in change_dict if key not in self.SECTION_ENTRY_NAMES] + for section_key in known_keys + extra_keys: + section_dict = change_dict[section_key] + name = self.SECTION_ENTRY_NAMES_PLURAL.get(section_key, section_key) + line_endings = "**" if use_markdown else "" + final_strings.append(f"{line_endings}{name}:{line_endings}") + if use_markdown: final_strings.append("") + for item in section_dict: + change, tag, change_type = item["change"], item["tag"], item["change_type"] + final_strings.append(f"{line_prefix}{tag} ({change_type}): {change}") + final_strings.append("") return "\n".join(final_strings) - def compare_differences(self, attribute_filter=None, title=""): + def compare_differences(self, attribute_filter=None, title="", use_markdown=True): """Compare two schemas and return a formatted report of all differences. Convenience method that combines gather_schema_changes() and pretty_print_change_dict() to produce a complete, human-readable comparison report in one call. If no title is - provided, generates a descriptive title from the schema names. + provided, generates a descriptive title from the schema names. In markdown mode the result + is the layout of hed-schemas' ``prerelease/PRERELEASE_CHANGES.md`` and can be written to + that file as is. Parameters: attribute_filter (HedKey or None): If provided, only entries with this attribute are @@ -338,22 +352,23 @@ def compare_differences(self, attribute_filter=None, title=""): Default is None. title (str): Custom title for the report. If empty string (default), generates a title like "Differences between SchemaName1 and SchemaName2". + use_markdown (bool): Passed to pretty_print_change_dict. Default is True. Returns: - str: Formatted markdown string describing all differences between the schemas. - Suitable for printing, saving to changelog files, or displaying in documentation. + str: Formatted string describing all differences between the schemas. Empty if the + schemas do not differ. Example: >>> report = comparer.compare_differences() >>> print(report) >>> # Or save to file - >>> with open("CHANGELOG.md", "a") as f: + >>> with open("PRERELEASE_CHANGES.md", "w") as f: ... f.write(report) """ changelog = self.gather_schema_changes(attribute_filter=attribute_filter) if not title: title = f"Differences between {self.schema1.name} and {self.schema2.name}" - return self.pretty_print_change_dict(changelog, title=title) + return self.pretty_print_change_dict(changelog, title=title, use_markdown=use_markdown) # Private helper methods @@ -428,7 +443,8 @@ def _add_removed_items(self, change_dict, not_in_2): Processes entries that exist in schema1 but have been removed from schema2. Categorizes removals as Major severity for Tags (breaking changes) and Unknown - severity for other section types. + severity for other section types, except a unit that schema2 still derives (for + example uV once V takes SI modifiers), which is a Patch. Parameters: change_dict (defaultdict): Change dictionary to append change entries to. @@ -436,12 +452,45 @@ def _add_removed_items(self, change_dict, not_in_2): Format: {section_key -> {name -> entry, ...}} """ for section_key, section in not_in_2.items(): - for tag, _ in section.items(): + for tag, entry in section.items(): type_name = self.SECTION_ENTRY_NAMES_PLURAL[section_key] change_type = "Major" if section_key == HedSectionKey.Tags else "Unknown" - change_dict[section_key].append( - {"change_type": change_type, "change": f"Tag {tag} deleted from {type_name}", "tag": tag} - ) + change = f"Tag {tag} deleted from {type_name}" + if section_key == HedSectionKey.Units: + derivation = self._still_derivable(tag, self._unit_class_in_schema2(entry)) + if derivation: + change_type = "Patch" + change = f"Unit {tag} deleted from {type_name}; {derivation}" + change_dict[section_key].append({"change_type": change_type, "change": change, "tag": tag}) + + def _unit_class_in_schema2(self, unit_entry): + """Return schema2's unit class entry with the same name as *unit_entry*'s class, or None.""" + unit_class_entry = getattr(unit_entry, "unit_class_entry", None) + if unit_class_entry is None: + return None + return self.schema2.unit_classes.get(unit_class_entry.name) + + @staticmethod + def _still_derivable(unit_name, unit_class_entry): + """Say how *unit_name* is still accepted by *unit_class_entry* (HED 8.5.0 drops listed SI variants). + + Parameters: + unit_name (str): The unit that was removed from the listing. + unit_class_entry (UnitClassEntry or None): The unit class in the newer schema. + + Returns: + str or None: ``"still derivable as u + V"`` when the name resolves through an SI modifier on a + listed unit, ``"still a form of volt"`` when it resolves without a modifier (a plural), + None when the newer schema does not accept the name at all. + """ + if unit_class_entry is None: + return None + derived = unit_class_entry.get_derivative_unit_entry(unit_name) + if derived is None or derived.name == unit_name: + return None + if unit_name.endswith(derived.name): + return f"still derivable as {unit_name[: -len(derived.name)]} + {derived.name}" + return f"still a form of {derived.name}" @staticmethod def _add_added_items(change_dict, not_in_1): @@ -512,12 +561,13 @@ def _add_misc_section_changes(change_dict, section_key, changes): ) change_dict[section_key].append({"change_type": change_type, "change": change_desc, "tag": misc_section}) - @staticmethod - def _add_unit_classes_changes(change_dict, section_key, entry1, entry2): + @classmethod + def _add_unit_classes_changes(cls, change_dict, section_key, entry1, entry2): """Add changes in unit class definitions to the change dictionary. Compares the units contained in two unit class entries and records additions/removals. - Unit removals are Major severity (breaking), unit additions are Patch severity. + Unit removals are Major severity (breaking) unless the newer class still derives the unit + from a listed one (uV as u + V), which is a Patch; unit additions are Patch severity. Parameters: change_dict (defaultdict): Change dictionary to append to. @@ -527,9 +577,12 @@ def _add_unit_classes_changes(change_dict, section_key, entry1, entry2): """ for unit in entry1.units: if unit not in entry2.units: - change_dict[section_key].append( - {"change_type": "Major", "change": f"Unit {unit} removed from {entry1.name}", "tag": entry1.name} - ) + derivation = cls._still_derivable(unit, entry2) + change_type = "Patch" if derivation else "Major" + change = f"Unit {unit} removed from {entry1.name}" + if derivation: + change += f"; {derivation}" + change_dict[section_key].append({"change_type": change_type, "change": change, "tag": entry1.name}) for unit in entry2.units: if unit not in entry1.units: change_dict[section_key].append( diff --git a/hed/schema/schema_validation/compliance_summary.py b/hed/schema/schema_validation/compliance_summary.py index 50e7382b..519367e0 100644 --- a/hed/schema/schema_validation/compliance_summary.py +++ b/hed/schema/schema_validation/compliance_summary.py @@ -168,7 +168,6 @@ def get_summary(self, verbose=True): lines.append(" - Missing descriptions on entries") lines.append(" - SuggestedTag/RelatedTag existence (8.3+ schemas)") lines.append(" - Unit class must have at least one unit") - lines.append(" - DefaultUnits must be in the tag's own unit classes") lines.append(" - HedID uniqueness across entries") lines.append(" - HedID completeness (all entries should have IDs)") lines.append(" - Attributes must have exactly one range type") diff --git a/hed/scripts/schema_script_util.py b/hed/scripts/schema_script_util.py index b29c85cc..964253e0 100644 --- a/hed/scripts/schema_script_util.py +++ b/hed/scripts/schema_script_util.py @@ -363,7 +363,9 @@ def _get_schema_comparison(schema, schema_reload, file_path, file_format): "If the problem is in the schema file, " "the following comparison should indicate the approximate source of the issues:" ) - error_text += "\n" + SchemaComparer(schema, schema_reload).compare_differences(title=title_prompt) + error_text += "\n" + SchemaComparer(schema, schema_reload).compare_differences( + title=title_prompt, use_markdown=False + ) return [error_text] return [] diff --git a/tests/schema/test_schema_attribute_validators.py b/tests/schema/test_schema_attribute_validators.py index 7f36ae97..c0a27d7a 100644 --- a/tests/schema/test_schema_attribute_validators.py +++ b/tests/schema/test_schema_attribute_validators.py @@ -4,6 +4,7 @@ from hed import load_schema_version from hed.schema import HedSectionKey from hed.schema.schema_validation import attribute_validators as schema_attribute_validators +from tests.schema import util_create_schemas class Test(unittest.TestCase): @@ -109,6 +110,21 @@ def test_unit_exists(self): tag_entry.attributes["defaultUnits"] = "bad_unit" self.assertTrue(schema_attribute_validators.unit_exists(self.hed_schema, tag_entry, attribute_name)) + def test_unit_exists_derived_default(self): + # HED 8.5.0 lets defaultUnits be a derived form (mV of a listed V); an unlisted unit still fails. + schema = util_create_schemas.load_schema_derived_default() + unit_class = schema.unit_classes["testVoltageUnits"] + self.assertEqual(unit_class.attributes["defaultUnits"], "mV") + self.assertEqual(schema_attribute_validators.unit_exists(schema, unit_class, "defaultUnits"), []) + + for bad_default in ("mX", "mv", "kmV", "millivolts"): + unit_class = copy.deepcopy(unit_class) + unit_class.attributes["defaultUnits"] = bad_default + issues = schema_attribute_validators.unit_exists(schema, unit_class, "defaultUnits") + self.assertEqual(len(issues), 1, bad_default) + self.assertEqual(issues[0]["code"], "SCHEMA_ATTRIBUTE_VALUE_INVALID", bad_default) + self.assertIn(f"invalid defaultUnit '{bad_default}'", issues[0]["message"], bad_default) + def test_deprecatedFrom(self): tag_entry = self.hed_schema.tags["Event/Measurement-event"] attribute_name = "deprecatedFrom" diff --git a/tests/schema/test_schema_compare.py b/tests/schema/test_schema_compare.py index b191ef01..14aeb236 100644 --- a/tests/schema/test_schema_compare.py +++ b/tests/schema/test_schema_compare.py @@ -5,7 +5,7 @@ import pandas as pd from hed import load_schema, load_schema_version -from hed.schema import HedKey, HedSectionKey +from hed.schema import HedKey, HedSectionKey, from_string from hed.schema.schema_comparer import SchemaComparer from hed.schema.schema_io.df_constants import EXTERNAL_ANNOTATION_KEY, PREFIXES_KEY, SOURCES_KEY from tests.schema import util_create_schemas @@ -421,6 +421,60 @@ def test_multiple_key_columns(self): self.assertIn("Column values differ", messages) +class TestDerivableUnitRemoval(unittest.TestCase): + """HED 8.5.0 drops listed SI variants (uV) that stay valid through modifiers: Patch, not Major.""" + + @classmethod + def setUpClass(cls): + # Remove uV (still derivable as u + V) and mph (not derivable) from 8.4.0 by editing the mediawiki + # text and reloading, so the derived-unit maps are rebuilt for the smaller unit classes. + cls.schema1 = load_schema_version("8.4.0") + lines = cls.schema1.get_as_mediawiki_string().split("\n") + kept = [line for line in lines if not line.startswith("** uV ") and not line.startswith("** mph ")] + if len(kept) != len(lines) - 2: + raise AssertionError(f"expected to drop exactly the uV and mph lines, dropped {len(lines) - len(kept)}") + cls.schema2 = from_string("\n".join(kept), schema_format=".mediawiki") + cls.changes = SchemaComparer(cls.schema1, cls.schema2).gather_schema_changes() + + def _changes_for(self, section_key, tag): + return [item for item in self.changes[section_key] if item["tag"] == tag] + + def test_derivable_unit_removal_is_patch_in_unit_class(self): + items = [ + item + for item in self._changes_for(HedSectionKey.UnitClasses, "electricPotentialUnits") + if "uV" in item["change"] + ] + self.assertEqual(len(items), 1) + self.assertEqual(items[0]["change_type"], "Patch") + self.assertEqual(items[0]["change"], "Unit uV removed from electricPotentialUnits; still derivable as u + V") + + def test_derivable_unit_removal_is_patch_in_units_section(self): + items = self._changes_for(HedSectionKey.Units, "uV") + self.assertEqual(len(items), 1) + self.assertEqual(items[0]["change_type"], "Patch") + self.assertEqual(items[0]["change"], "Unit uV deleted from Units; still derivable as u + V") + + def test_non_derivable_unit_removal_unchanged(self): + class_items = [ + item for item in self._changes_for(HedSectionKey.UnitClasses, "speedUnits") if "mph" in item["change"] + ] + self.assertEqual(len(class_items), 1) + self.assertEqual(class_items[0]["change_type"], "Major") + self.assertEqual(class_items[0]["change"], "Unit mph removed from speedUnits") + unit_items = self._changes_for(HedSectionKey.Units, "mph") + self.assertEqual(len(unit_items), 1) + self.assertEqual(unit_items[0]["change_type"], "Unknown") + self.assertEqual(unit_items[0]["change"], "Tag mph deleted from Units") + + def test_still_derivable_wording(self): + electric = self.schema2.unit_classes["electricPotentialUnits"] + self.assertEqual(SchemaComparer._still_derivable("uV", electric), "still derivable as u + V") + self.assertEqual(SchemaComparer._still_derivable("volts", electric), "still a form of volt") + self.assertIsNone(SchemaComparer._still_derivable("UV", electric)) + self.assertIsNone(SchemaComparer._still_derivable("uV", None)) + + class TestPrettyPrintChangeDict(unittest.TestCase): """Tests for pretty_print_change_dict and compare_differences formatting.""" @@ -437,9 +491,62 @@ def test_empty_dict_returns_empty_string(self): def test_markdown_uses_bold_headers_and_bullets(self): result = self.comp.pretty_print_change_dict(self.changes, use_markdown=True) self.assertIn("**", result) - self.assertIn(" - ", result) + self.assertIn("\n- ", result) self.assertNotIn("\t", result) + def test_markdown_layout_matches_prerelease_changes(self): + # hed-schemas prerelease/PRERELEASE_CHANGES.md: "## title", blank line, "**Section:**", blank line, + # "- " bullets, blank line before the next section, single trailing newline. + two_sections = { + HedSectionKey.Tags: [ + {"change_type": "Minor", "change": "Item Consume added", "tag": "Consume"}, + {"change_type": "Patch", "change": "Description of Event modified", "tag": "Event"}, + ], + HedSectionKey.Units: [{"change_type": "Minor", "change": "Item ampere added", "tag": "ampere"}], + } + result = self.comp.pretty_print_change_dict(two_sections, title="Differences between A and B") + self.assertEqual( + result, + "## Differences between A and B\n\n**Tags:**\n\n- Consume (Minor): Item Consume added\n" + "- Event (Patch): Description of Event modified\n\n**Units:**\n\n- ampere (Minor): Item ampere added\n", + ) + lines = result.split("\n") + self.assertEqual(lines[0], "## Differences between A and B") + self.assertEqual(lines[1], "") + self.assertTrue(result.endswith("\n")) + self.assertFalse(result.endswith("\n\n")) + headers = [index for index, line in enumerate(lines) if line.startswith("**") and line.endswith(":**")] + self.assertGreater(len(headers), 1) + for index in headers: + self.assertEqual(lines[index + 1], "", "blank line after section header") + self.assertTrue(lines[index + 2].startswith("- "), "bullets start right after the blank line") + if index > 0: + self.assertEqual(lines[index - 1], "", "blank line before section header") + for line in lines: + self.assertFalse(line.startswith(" - "), "no leading space before bullets") + + def test_sections_follow_section_entry_names_order(self): + # A dict built elsewhere may list sections in any order; output follows SECTION_ENTRY_NAMES, then extras. + reversed_dict = { + "SomethingElse": [{"change_type": "Unknown", "change": "x", "tag": "x"}], + HedSectionKey.Units: [{"change_type": "Minor", "change": "Item A added", "tag": "A"}], + HedSectionKey.Tags: [{"change_type": "Minor", "change": "Item B added", "tag": "B"}], + } + result = self.comp.pretty_print_change_dict(reversed_dict, title="") + self.assertLess(result.index("**Tags:**"), result.index("**Units:**")) + self.assertLess(result.index("**Units:**"), result.index("**SomethingElse:**")) + + def test_markdown_without_title_has_no_heading(self): + result = self.comp.pretty_print_change_dict(self.changes, title="") + self.assertTrue(result.startswith("**")) + self.assertNotIn("## ", result) + + def test_plain_text_title_is_bare(self): + result = self.comp.pretty_print_change_dict(self.changes, title="Plain title", use_markdown=False) + self.assertTrue(result.startswith("Plain title\n\n")) + self.assertNotIn("## ", result) + self.assertNotIn("**", result) + def test_plain_text_uses_tabs_no_bold(self): result = self.comp.pretty_print_change_dict(self.changes, use_markdown=False) self.assertNotIn("**", result) @@ -463,7 +570,9 @@ def test_extras_section_uses_human_readable_name(self): def test_custom_title_appears_in_output(self): result = self.comp.compare_differences(title="My Special Title") - self.assertIn("My Special Title", result) + self.assertTrue(result.startswith("## My Special Title\n\n")) + plain = self.comp.compare_differences(title="My Special Title", use_markdown=False) + self.assertTrue(plain.startswith("My Special Title\n\n")) def test_auto_generated_title_uses_schema_names(self): schema1 = load_schema_version("score_1.0.0")