Repository: BAMWelDX/weldx

Issue:
sort string list output in asdf tree
in some places we store lists of strings in asdf files, most for the CSM and related functions notably here:
https://github.com/BAMWelDX/weldx/blob/9ffdba4c3594fe9bd73f214005d2a6ab8314369d/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py#L83-L92

The sorting of these list items seem to change randomly when (re)creating files.
For consistency and better diffs it would be nice to sort these kind of lists before writing them to asdf.

Need to check if the order might be relevant to the implementation but I guess not for most cases?

Discussion:
pep8speaks: Hello @vhirtham! Thanks for opening this PR. 

* In the file [`weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py`](https://github.com/BAMWelDX/weldx/blob/4fd384e02773b572fa25a03182659f4237690c63/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py):

> [Line 474:26](https://github.com/BAMWelDX/weldx/blob/4fd384e02773b572fa25a03182659f4237690c63/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py#L474): [E231](https://duckduckgo.com/?q=pep8%20E231) missing whitespace after ':'
> [Line 474:26](https://github.com/BAMWelDX/weldx/blob/4fd384e02773b572fa25a03182659f4237690c63/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py#L474): [E999](https://duckduckgo.com/?q=pep8%20E999) SyntaxError: invalid syntax
> [Line 474:28](https://github.com/BAMWelDX/weldx/blob/4fd384e02773b572fa25a03182659f4237690c63/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py#L474): [E251](https://duckduckgo.com/?q=pep8%20E251) unexpected spaces around keyword / parameter equals

Do see the [Hitchhiker's guide to code style](https://goo.gl/hqbW4r)
codecov[bot]: # [Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) Report
> Merging [#430](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) (4fd384e) into [master](https://codecov.io/gh/BAMWelDX/weldx/commit/d29a2de0c74264db977ffd21677c823690133663?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) (d29a2de) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/BAMWelDX/weldx/pull/430/graphs/tree.svg?width=650&height=150&src=pr&token=wdof1qQTsn&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)

```diff
@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   97.25%   97.25%           
=======================================
  Files          87       87           
  Lines        5388     5391    +3     
=======================================
+ Hits         5240     5243    +3     
  Misses        148      148           
```


| [Impacted Files](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX) | Coverage Δ | |
|---|---|---|
| [...ore/transformations/coordinate\_system\_hierarchy.py](https://codecov.io/gh/BAMWelDX/weldx/pull/430/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX#diff-d2VsZHgvYXNkZi90YWdzL3dlbGR4L2NvcmUvdHJhbnNmb3JtYXRpb25zL2Nvb3JkaW5hdGVfc3lzdGVtX2hpZXJhcmNoeS5weQ==) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX). Last update [d29a2de...4fd384e](https://codecov.io/gh/BAMWelDX/weldx/pull/430?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BAMWelDX).

vhirtham: I adjusted the CSM serialization and our `weldx.asdf.util.dataclass_serialization_class` function. That should cover most cases. Didn't find any further corresponding type hints in the asdf directory and searching for

~~~
type: array
    items:
      type: string
~~~

in the schemas only brought up an additional case in the CSM I missed initially. I guess we covered everything.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 28c2458..55e6bbf 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -21,6 +21,9 @@
 
 ### ASDF
 
+- sort `List[str]` before serialization of most `weldx` classes to avoid random reordering in the same file and enforce
+  consistency. [[#430]](https://github.com/BAMWelDX/weldx/pull/430)
+
 ### deprecations
 
 - `lcs_coords_from_ts` will be removed in version 0.5.0 [[#426]](https://github.com/BAMWelDX/weldx/pull/426)
diff --git a/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py b/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py
index 8e6a87c..7b904ae 100644
--- a/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py
+++ b/weldx/asdf/tags/weldx/core/transformations/coordinate_system_hierarchy.py
@@ -91,6 +91,11 @@ class CoordinateSystemManagerSubsystem:
     subsystems: List[str]
     members: List[str]
 
+    def __post_init__(self):
+        """Sort the string lists."""
+        self.subsystems = sorted(self.subsystems)
+        self.members = sorted(self.members)
+
 
 class CoordinateSystemManagerSubsystemASDF(WeldxType):
     """Serialization class for a CoordinateSystemManagerSubsystem instance"""
diff --git a/weldx/asdf/util.py b/weldx/asdf/util.py
index ef2efee..e81bf82 100644
--- a/weldx/asdf/util.py
+++ b/weldx/asdf/util.py
@@ -299,6 +299,7 @@ def dataclass_serialization_class(
     to_tree_mod: Callable = None,
     from_tree_mod: Callable = None,
     validators: dict = None,
+    sort_string_lists: bool = True,
 ) -> Type:
     """Generate a asdf serialization class for a python dataclass.
 
@@ -338,6 +339,17 @@ def dataclass_serialization_class(
     if from_tree_mod is None:
         from_tree_mod = _noop
 
+    if sort_string_lists:
+        original_to_tree_mod = to_tree_mod
+
+        def _sort_string_list(tree):
+            for k, v in tree.items():
+                if isinstance(v, list) and all(isinstance(item, str) for item in v):
+                    tree[k] = sorted(v)
+            return original_to_tree_mod(tree)
+
+        to_tree_mod = _sort_string_list
+
     class _SerializationClass(WeldxType):
         name = class_name
         version = v


Test patch:
diff --git a/weldx/tests/asdf_tests/test_asdf_util.py b/weldx/tests/asdf_tests/test_asdf_util.py
index 4497ff0..4369333 100644
--- a/weldx/tests/asdf_tests/test_asdf_util.py
+++ b/weldx/tests/asdf_tests/test_asdf_util.py
@@ -1,8 +1,11 @@
 """tests for asdf utility functions."""
+from dataclasses import dataclass
+from typing import List
+
 import pytest
 
 from weldx import WeldxFile
-from weldx.asdf.util import get_yaml_header
+from weldx.asdf.util import dataclass_serialization_class, get_yaml_header
 
 
 @pytest.fixture(scope="function")
@@ -40,3 +43,85 @@ def test_get_yaml_header(create_file_and_buffer, index, parse):
     else:
         assert isinstance(header, str)
         assert "asdf_library" in header
+
+
+def _to_tree_mod(tree):
+    tree["a"] += ["d"]
+    return tree
+
+
+def _from_tree_mod(tree):
+    tree["a"] += ["e"]
+    return tree
+
+
+@pytest.mark.parametrize(
+    "val_a, exp_val_a_tree, exp_val_a_dc, to_tree_mod, from_tree_mod,"
+    "sort_string_lists",
+    [
+        (["c", "b", "a"], ["a", "b", "c"], ["a", "b", "c"], None, None, True),
+        (["c", "b", "a"], ["c", "b", "a"], ["c", "b", "a"], None, None, False),
+        # not a pure string list -> no sorting
+        (["c", 1, "a"], ["c", 1, "a"], ["c", 1, "a"], None, None, True),
+        (["c", 1, "a"], ["c", 1, "a"], ["c", 1, "a"], None, None, False),
+        # check to_tree_mod is called
+        (["c", "b"], ["b", "c", "d"], ["b", "c", "d"], _to_tree_mod, None, True),
+        (["c", "b"], ["c", "b", "d"], ["c", "b", "d"], _to_tree_mod, None, False),
+        # check_from_tree_mod is called
+        (["c"], ["c", "d"], ["c", "d", "e"], _to_tree_mod, _from_tree_mod, False),
+        (["c"], ["c"], ["c", "e"], None, _from_tree_mod, False),
+    ],
+)
+def test_dataclass_serialization_class(
+    val_a, exp_val_a_tree, exp_val_a_dc, to_tree_mod, from_tree_mod, sort_string_lists
+):
+    """Test the `dataclass_serialization_class` function.
+
+    The test defines a dataclass and its corresponding serialization class using
+    `dataclass_serialization_class`. It first calls to_tree to get tree from the
+    generated serialization class. Afterwards the tree is used with the `from_tree`
+    method to construct a new dataclass instance. The results of the function calls are
+    checked against the expected values.
+
+    Parameters
+    ----------
+    val_a :
+        Initial value of the dataclasses' variable a
+    exp_val_a_tree :
+        Expected value of the variable a in the tree after `to_tree` was run
+    exp_val_a_dc :
+        Expected value of the variable a of the reconstructed dataclass
+    to_tree_mod :
+        The value passed as corresponding function parameter
+    from_tree_mod :
+        The value passed as corresponding function parameter
+    sort_string_lists
+        The value passed as corresponding function parameter
+
+
+    """
+
+    @dataclass
+    class _DataClass:
+        a: List[str]
+        b: int = 1
+
+    dc = _DataClass(a=val_a, b=2)
+
+    dataclass_asdf = dataclass_serialization_class(
+        class_type=_DataClass,
+        class_name="Test",
+        version="1.0.0",
+        sort_string_lists=sort_string_lists,
+        to_tree_mod=to_tree_mod,
+        from_tree_mod=from_tree_mod,
+    )
+    tree = dataclass_asdf.to_tree(dc, None)
+
+    assert tree["b"] == 2
+    assert tree["a"] == exp_val_a_tree
+
+    dc_restored = dataclass_asdf.from_tree(tree, None)
+
+    assert dc_restored.b == 2
+    assert dc_restored.a == exp_val_a_dc