fix: correct validate_inputs error message when both missing and additional inputs exist#760
Conversation
…tional inputs exist
In burr/core/action.py, the validate_inputs() method had a bug in its
error message construction due to operator precedence in a nested ternary
expression.
The original code:
raise ValueError(
f"Inputs to function {self} are invalid. "
+ f"Missing the following inputs: ..."
if missing_inputs
else (...)
)
Due to Python operator precedence, the ternary 'if' binds lower than '+',
so the whole expression is evaluated as:
(str1 + str2) if missing_inputs else (str3 if additional_inputs else "")
This means when BOTH missing_inputs and additional_inputs are non-empty,
the error message only reports the missing inputs and silently drops info
about the additional inputs. Similarly, when only additional_inputs exist,
the 'Inputs to function ... are invalid' prefix is omitted.
Fixed by building error message parts independently:
- Always include the 'Inputs to function ... are invalid' prefix
- Conditionally append missing inputs info
- Conditionally append additional inputs info
- Join all parts with spaces
This ensures all relevant validation errors are reported regardless of
which combination of issues exist.
There was a problem hiding this comment.
Pull request overview
Fixes incorrect validate_inputs() error message construction in burr/core/action.py caused by Python conditional-expression operator precedence, ensuring the invalid-inputs prefix is always included and that both missing and additional inputs are reported when present.
Changes:
- Replaced nested conditional-expression string building with incremental
partsconstruction. - Ensured missing/additional input details are appended independently (and standardized comma spacing for additional inputs).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parts.append(f"Missing the following inputs: {', '.join(missing_inputs)}.") | ||
| if additional_inputs: | ||
| parts.append(f"Additional inputs: {', '.join(additional_inputs)}.") |
| if missing_inputs or additional_inputs: | ||
| raise ValueError( | ||
| f"Inputs to function {self} are invalid. " | ||
| + f"Missing the following inputs: {', '.join(missing_inputs)}." | ||
| if missing_inputs | ||
| else ( | ||
| "" f"Additional inputs: {','.join(additional_inputs)}." | ||
| if additional_inputs | ||
| else "" | ||
| ) | ||
| ) | ||
| parts = [f"Inputs to function {self} are invalid."] | ||
| if missing_inputs: | ||
| parts.append(f"Missing the following inputs: {', '.join(missing_inputs)}.") | ||
| if additional_inputs: | ||
| parts.append(f"Additional inputs: {', '.join(additional_inputs)}.") | ||
| raise ValueError(" ".join(parts)) |
|
Friendly ping for review 🙏 This is a small, focused fix. CI is passing. Thank you! |
|
Hi! 👋 This PR has passed all CI checks and looks ready for review. Could a maintainer please take a look when they have a chance? Thank you! |
skrawcz
left a comment
skrawcz
left a comment
There was a problem hiding this comment.
LGTM — good catch on the operator precedence bug. The old code parsed as (prefix + missing_msg) if missing_inputs else ... which silently dropped the additional_inputs message when both were present and lost the prefix when only additional_inputs was set. The new code with explicit list-building is clearer and correct.
One optional suggestion: since there are no existing tests for validate_inputs, this would be a great spot to add a few (happy path, missing only, additional only, both). But not blocking — the fix itself is straightforward.
Problem
In
burr/core/action.py, thevalidate_inputs()method had a bug in its error message construction due to Python's operator precedence in a nested ternary expression.The original code:
Due to Python operator precedence,
if...else(conditional expression) binds lower than+, so the expression is actually parsed as:This causes two bugs:
When both
missing_inputsandadditional_inputsare non-empty: Theif missing_inputsbranch is taken, and only the missing inputs info is reported. The additional inputs info is silently dropped.When only
additional_inputsis non-empty: The error message is just"Additional inputs: ..."without the"Inputs to function {self} are invalid."prefix.Fix
Replaced the nested ternary with clear, independent conditionals that build error message parts:
This ensures:
"Inputs to function ... are invalid."prefix is always present','.joinchanged to', '.joinfor consistent spacing