Fix mixin arg merge order for lists and scalars - #71
Merged
Conversation
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.
Co-authored-by: Kimberly N. McGuire <kimberleymcguire@gmail.com>
Remove comment about command line arguments order.
PiousCrossten
changed the title
cottsay
approved these changes
Aug 21, 2026
cottsay
changed the title
cottsay
pushed a commit
that referenced
this pull request
Aug 21, 2026
* 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. * Apply suggestion from @knmcguire Co-authored-by: Kimberly N. McGuire <kimberleymcguire@gmail.com> * Remove unnecessary comment in mixin_argument.py Remove comment about command line arguments order. * Update test_mixin_argument.py --------- Co-authored-by: Kimberly N. McGuire <kimberleymcguire@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of #59, opened from a branch in this repository. Same four commits, unchanged the review discussion and approval are on #59.
Fixes #40.
The problem
_update_args()merges a mixin's list values by prepending them to the existing list. Applying the selected mixins in command line order therefore produced a final list in reverse of what the user asked for:colcon build --mixin a b
...ended up with
b's values ahead ofa's.The change
MixinArgumentDecorator.parse_args()now runs two passes overargs.mixin:_update_args(), leaves the merged list in command line order while explicit command line arguments still land last and therefore win.Splitting the loop in two is what keeps the error message order and the merge order independent of each other.
As a side effect, a later mixin now also overrides a scalar value set by an earlier one.
Tests
New
test/test_mixin_argument.py- 11 tests covering list ordering for two and three mixins, reversed selection, command line precedence, single/no mixin, scalar override and precedence, and the unavailable-mixin error naming the first offender.