fix: correct inverted preserve_order docstring in resolve_matching_names functions#5310
fix: correct inverted preserve_order docstring in resolve_matching_names functions#5310sellai-youcef wants to merge 3 commits intoisaac-sim:mainfrom
Conversation
Signed-off-by: sellai-youcef <sellaiyoucefs@gmail.com>
Greptile SummaryThis PR corrects the inverted
Confidence Score: 3/5Not safe to merge — the primary fix is incomplete and introduces a formatting defect. Two P1 issues remain: a missing indentation that will break Sphinx doc rendering, and the sibling function
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["preserve_order=False (default)"] --> B["Loop over list_of_strings\n(target order)"]
B --> C["Append matches in\ntarget string order"]
C --> D["Return indices & names\nin target-string order"]
E["preserve_order=True"] --> F["Loop over list_of_strings\n(target order)"]
F --> G["Reorder by key_index\n(query/regex order)"]
G --> H["Return indices & names\nin query-key order"]
|
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order | ||
| of the provided list of query regular expressions. |
There was a problem hiding this comment.
Missing indentation on
preserve_order=True line
The first line of the preserve_order is True paragraph lost its 4-space indentation during the edit, breaking the docstring's visual alignment and likely causing RST/Sphinx rendering issues (the line will not be treated as a continuation of the same paragraph).
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order | |
| of the provided list of query regular expressions. | |
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order | |
| of the provided list of query regular expressions. |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR fixes the inverted preserve_order docstring descriptions in resolve_matching_names, where the prose for True and False were swapped relative to the actual code behavior. The fix is correct — I traced through the implementation and confirmed: when preserve_order=True, the reorder block groups results by key_index order (query key order), and when False, the outer loop over list_of_strings naturally yields target-string order. The examples in the docstring were already correct and remain consistent with the updated prose.
However, there are two issues that need addressing before merge.
Verdict
Minor fixes needed
The docstring correction is accurate, but there’s a missing-indent formatting issue on line 186 and — more importantly — the same inverted docstring bug in resolve_matching_names_values (lines 283–288) was not fixed, despite being explicitly called out in issue #5146. Both should be addressed in this PR.
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order | ||
| of the provided list of strings. This means that the ordering is dictated by the order of the target strings | ||
| and not the order of the query regular expressions. | ||
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order |
There was a problem hiding this comment.
🔴 Critical: Missing indentation — This line lost its leading 4-space indent, which will break Sphinx/docstring rendering. The rest of the docstring block uses 4-space indentation; this line must match.
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order | |
| If the :attr:`preserve_order` is True, the ordering of the matched indices and names is the same as the order |
|
|
||
| If the :attr:`preserve_order` is False, the ordering of the matched indices and names is the same as the order | ||
| of the provided list of query regular expressions. | ||
| of the provided list of strings. This means that the ordering is dictated by the order of the target strings |
There was a problem hiding this comment.
🟡 Warning: Incomplete fix — resolve_matching_names_values still has the same bug
The linked issue #5146 explicitly calls out that resolve_matching_names_values (lines 283–288) has the identical inverted docstring. The code logic in that function is the same (reorder block runs when preserve_order=True, grouping by query key order), but the docstring there still claims True → target string order and False → query key order — which is backwards.
Please apply the same swap to resolve_matching_names_values so the full issue is resolved in one PR.
…mes_values Signed-off-by: sellai-youcef <sellaiyoucefs@gmail.com>
Description
The
preserve_orderparameter descriptions were inverted in the docstringsof
resolve_matching_namesandresolve_matching_names_valuesinsource/isaaclab/isaaclab/utils/string.py.The code logic and examples were already correct — only the prose descriptions
were swapped.
Fixes #5146
Type of change
Checklist