Apply mixins in reverse order to preserve command line order (#40)

When multiple mixins provided list arguments (e.g. cmake-args) they were
concatenated in reverse of the order given on the command line: each
mixin prepends its values, and applying the mixins in forward order
stacked them backwards. Apply the mixins in reverse so the resulting
list follows the order the mixins were given while keeping any explicit
command line arguments last. As a side effect this also lets a later
mixin override a scalar value set by an earlier one.

The requested mixins are still validated in the given order so an error
reports the first unavailable mixin.

Add regression tests covering list ordering, scalar override and command
line precedence.

Author: PiousCrossten

colcon_mixin/mixin/mixin_argument.py

@@ -181,12 +181,20 @@ def collect_parsers_by_verb(root, parsers, parent_verbs=()):
         # update args based on selected mixins
         if 'mixin_verb' in args:
             mixins = mixins_by_verb.get(args.mixin_verb, {})
+            # validate the requested mixins in the order given on the command
+            # line so an error reports the first unavailable mixin
             for mixin in args.mixin or ():
                 if mixin not in mixins:
                     context = '.'.join(args.mixin_verb)
                     self._parser.error(
                         "Mixin '{mixin}' is not available for '{context}'"
                         .format_map(locals()))
+            # apply the mixins in reverse order: since _update_args() handles
+            # each mixin by prepending its list values, iterating in reverse
+            # makes the resulting list follow the order the mixins were given
+            # on the command line while keeping any explicit command line
+            # arguments last (see #40)
+            for mixin in reversed(args.mixin or ()):
                 mixin_args = mixins[mixin]
                 logger.debug(
                     "Using mixin '{mixin}': {mixin_args}".format_map(locals()))

test/spell_check.words

@@ -4,6 +4,8 @@ argparse
 basenames
 basepath
 blocklist
+capsys
+cmake
 colcon
 completers
 defaultdict
@@ -17,6 +19,7 @@ plugin
 prepending
 pydocstyle
 pytest
+readouterr
 rtype
 scspell
 setuptools

test/test_mixin_argument.py

@@ -0,0 +1,91 @@
+# Copyright 2025 Open Source Robotics Foundation, Inc.
+# Licensed under the Apache License, Version 2.0
+
+import argparse
+from unittest.mock import patch
+
+from colcon_mixin.mixin.mixin_argument import MixinArgumentDecorator
+import pytest
+
+MIXINS = {
+    'a': {'cmake-args': ['FOO=A']},
+    'b': {'cmake-args': ['FOO=B']},
+    'c': {'cmake-args': ['FOO=C']},
+}
+
+
+SCALAR_MIXINS = {
+    'a': {'build-base': 'BASE_A'},
+    'b': {'build-base': 'BASE_B'},
+}
+
+
+def _parse(argv, mixins=MIXINS):
+    base = argparse.ArgumentParser(prog='colcon')
+    decorator = MixinArgumentDecorator(base)
+    subparsers = decorator.add_subparsers(dest='verb')
+    build = subparsers.add_parser('build')
+    build.add_argument('--cmake-args', nargs='*', default=[])
+    build.add_argument('--build-base', default='build')
+    mixins_by_verb = {('build',): mixins}
+    with patch(
+        'colcon_mixin.mixin.mixin_argument.get_mixins',
+        return_value=mixins_by_verb,
+    ):
+        return decorator.parse_args(argv)
+
+
+def test_multiple_mixins_preserve_command_line_order():
+    # regression test for issue #40: the resulting list must follow the
+    # order the mixins were given on the command line
+    args = _parse(['build', '--mixin', 'a', 'b'])
+    assert args.cmake_args == ['FOO=A', 'FOO=B']
+
+
+def test_multiple_mixins_follow_reversed_selection():
+    args = _parse(['build', '--mixin', 'b', 'a'])
+    assert args.cmake_args == ['FOO=B', 'FOO=A']
+
+
+def test_three_mixins_preserve_order():
+    args = _parse(['build', '--mixin', 'a', 'b', 'c'])
+    assert args.cmake_args == ['FOO=A', 'FOO=B', 'FOO=C']
+
+
+def test_command_line_arguments_take_precedence():
+    # explicit command line arguments must come last so they win
+    args = _parse(['build', '--mixin', 'a', 'b', '--cmake-args=FOO=CLI'])
+    assert args.cmake_args == ['FOO=A', 'FOO=B', 'FOO=CLI']
+
+
+def test_single_mixin():
+    args = _parse(['build', '--mixin', 'a'])
+    assert args.cmake_args == ['FOO=A']
+
+
+def test_no_mixin():
+    args = _parse(['build'])
+    assert args.cmake_args == []
+
+
+def test_scalar_last_listed_mixin_wins():
+    # a scalar value from a later mixin must override an earlier one
+    args = _parse(['build', '--mixin', 'a', 'b'], SCALAR_MIXINS)
+    assert args.build_base == 'BASE_B'
+
+
+def test_scalar_reversed_selection():
+    args = _parse(['build', '--mixin', 'b', 'a'], SCALAR_MIXINS)
+    assert args.build_base == 'BASE_A'
+
+
+def test_scalar_command_line_argument_take_precedence():
+    args = _parse(
+        ['build', '--mixin', 'a', 'b', '--build-base', 'CLI'], SCALAR_MIXINS)
+    assert args.build_base == 'CLI'
+
+
+def test_unavailable_mixin_reports_first(capsys):
+    with pytest.raises(SystemExit):
+        _parse(['build', '--mixin', 'bad', 'a'])
+    assert "Mixin 'bad' is not available" in capsys.readouterr().err