Fix/unit duplication check

[jdacoello]

Solves issue #483 . It does solve it at the vss-tools level, which means that the validation occurs for every exporter.

[sschleemilch]

description was a bit misleading for me. I see it as a a long version of the key, kind of an alias. But not sure what the initial intention was behind having the embedded "unit" at all

[jdacoello]

It seems to be a label of the unit.

[sschleemilch]

I don't quite get why it needs to be only unique within one quantity since you can either use the key or the unit to reference a unit from the model. Therefore it needs to be globally unique. Therefore this can be simplified:

def check_unique_units(units: dict[str, VSSUnit]) -> None:
    # <unit>: <first-key-using-it>
    unit_defs: dict[str, str] = {}
    for key, value in units.items():
        unit = value.unit
        if not unit:
            continue
        if unit in unit_defs:
            log.error(f"Duplicated unit: '{value.unit}'. Used by: {[unit_defs[unit], key]}")
            raise DuplicatedUnitException()
        unit_defs[unit] = key

Which will cause a log error:

ERROR    Duplicated unit: 'kilometers per hour'. Used by: ['km/h', 'km/x']

units.yaml

km/h:
  unit: kilometers per hour
  definition: Velocity
  quantity: velocity
  allowed-datatypes:
    - numeric

km/x:
  unit: kilometers per hour
  definition: Velocity
  quantity: velocity
  allowed-datatypes:
    - numeric

[sschleemilch]

While we are at it, we probably should improve the raising and logging. We should only raise and put the logging statement into it I guess:

diff --git i/src/vss_tools/units_quantities.py w/src/vss_tools/units_quantities.py
index 3d2ba39..3cae468 100644
--- i/src/vss_tools/units_quantities.py
+++ w/src/vss_tools/units_quantities.py
@@ -18,6 +18,10 @@ class MalformedDictException(Exception):
     pass
 
 
+class DuplicatedUnitException(Exception):
+    pass
+
+
 def load_units_or_quantities(
     files: list[Path], class_type: type[VSSUnit | VSSQuantity]
 ) -> dict[str, VSSUnit | VSSQuantity]:
@@ -30,8 +34,7 @@ def load_units_or_quantities(
         log.info(f"Loaded '{class_type.__name__}', file={file.absolute()}, elements={len(content)}")
         for k, v in content.items():
             if v is None:
-                log.error(f"'{class_type.__name__}', '{k}' is 'None'")
-                raise MalformedDictException()
+                raise MalformedDictException(f"'{class_type.__name__}', '{k}' is 'None'")
             overwrite = False
             if k in data:
                 overwrite = True
@@ -51,8 +54,22 @@ def load_units_or_quantities(
 
 
 def load_units(unit_files: list[Path]) -> dict[str, VSSUnit]:
-    return load_units_or_quantities(unit_files, VSSUnit)  # type: ignore[return-value]
+    units: dict[str, VSSUnit] = load_units_or_quantities(unit_files, VSSUnit)  # type: ignore[return-value]
+    check_unique_units(units)
+    return units
 
 
 def load_quantities(quantities: list[Path]) -> dict[str, VSSQuantity]:
     return load_units_or_quantities(quantities, VSSQuantity)  # type: ignore[return-value]
+
+
+def check_unique_units(units: dict[str, VSSUnit]) -> None:
+    # def: <first-key-using-it>
+    unit_defs: dict[str, str] = {}
+    for key, value in units.items():
+        unit = value.unit
+        if not unit:
+            continue
+        if unit in unit_defs:
+            raise DuplicatedUnitException(f"Duplicated unit: '{value.unit}'. Used by: {[unit_defs[unit], key]}")
+        unit_defs[unit] = key

And then in main catch those to post a last critical log before exiting:

diff --git i/src/vss_tools/main.py w/src/vss_tools/main.py
index ec47e13..02c9dbd 100644
--- i/src/vss_tools/main.py
+++ w/src/vss_tools/main.py
@@ -24,7 +24,7 @@ from vss_tools.model import (
 )
 from vss_tools.strict import StrictExceptions, StrictOption, load_strict_exceptions
 from vss_tools.tree import ModelValidationException, VSSNode, add_struct_schemas, build_tree
-from vss_tools.units_quantities import load_quantities, load_units
+from vss_tools.units_quantities import load_quantities, load_units, DuplicatedUnitException, MalformedDictException
 from vss_tools.vspec import InvalidSpecDuplicatedEntryException, InvalidSpecException, load_vspec
 
 
@@ -201,7 +201,7 @@ def get_trees(
         log.info(f"User defined extra attributes: {extended_attributes}")
     try:
         load_quantities_and_units(quantities, units, vspec.parent)
-    except ModelValidationException as e:
+    except (ModelValidationException, DuplicatedUnitException, MalformedDictException) as e:
         log.critical(e)
         exit(1)

That probably will be a bit nicer for the end user without a stack trace:

[jdacoello]

Considered the comments and suggestions and updated the PR with the latest commit.

[erikbosch]

LGTM

[erikbosch]

MoM: Merge