Enhancement | Issue #21 | OPT-1 | Implement typed argument casting #306
Conversation
WalkthroughA new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Parsing
participant Casting
User->>Parsing: Provide argument/config string values
Parsing->>Casting: Call cast_value(value)
Casting-->>Parsing: Return casted value (bool/int/float/str)
Parsing-->>User: Return parsed/casted arguments or config
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
exequielrafaela
requested review from
Franr,
angelofenoglio and
diego-ojeda-binbash
Pull Request Test Coverage Report for Build 15472119070Details
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
leverage/_casting.py (1)
8-26: Consider expanding boolean detection and adding input validation.The current implementation has a few areas for improvement:
Limited boolean detection: Only handles "true"/"false" but common boolean representations like "yes"/"no", "1"/"0", "on"/"off" are not supported.
Missing input validation: The function expects a string but doesn't validate the input type.
Whitespace handling: Consider stripping whitespace before processing.
Consider this enhanced version:
def cast_value(value: str) -> Any: """Try to cast a string to bool, int or float. Args: value (str): Value to cast. Returns: Any: The value converted to its apparent type or the original string. """ + if not isinstance(value, str): + return value + + value = value.strip() + if not value: + return value + lowered = value.lower() - if lowered in {"true", "false"}: + if lowered in {"true", "false", "yes", "no", "1", "0", "on", "off"}: + return lowered in {"true", "yes", "1", "on"} - return lowered == "true" try: return int(value) except ValueError: try: return float(value) except ValueError: return valueleverage/conf.py (1)
39-39: Consider updating the TODO comment.The TODO comment mentions leveraging type casting utilities, which is now partially implemented with the
cast_valueintegration. Consider updating this comment to reflect the current state.- # TODO: Return an Env object instead of a dictionary, to be able to leverage its type casting utilities + # TODO: Consider returning an Env object instead of a dictionary for enhanced type casting capabilities
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
leverage/_casting.py(1 hunks)leverage/_parsing.py(2 hunks)leverage/conf.py(2 hunks)tests/test__parsing.py(1 hunks)tests/test_conf.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
leverage/conf.py (1)
leverage/_casting.py (1)
cast_value(8-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: integration_tests_cli_refarch (3.9.15, 1.3.5-0.2.0)
🔇 Additional comments (7)
leverage/conf.py (2)
9-9: Clean integration of the casting utility.The import is properly placed and follows the project's import organization.
61-61: Effective integration of value casting in config loading.The casting is applied appropriately during value storage. Since
cast_valuesafely returns the original value if casting fails, this integration is robust and won't introduce errors.leverage/_parsing.py (3)
5-5: Consistent import of the casting utility.The import follows the same pattern used in other modules and is properly placed.
45-45: Proper casting integration for positional arguments.The casting is applied after stripping whitespace but before adding to the args list, ensuring clean typed values.
52-52: Correct casting integration for keyword argument values.The casting is appropriately applied only to the value (not the key) of keyword arguments, maintaining the expected behavior while ensuring type consistency.
tests/test_conf.py (1)
33-33: Correct test updates to reflect the new casting behavior.The test expectations are properly updated to expect boolean
Truevalues instead of string"true"values, which aligns with the newcast_valueintegration in the configuration loading. This correctly validates that environment variables likeMFA_ENABLEDandDEBUGare now cast to their appropriate boolean types.Also applies to: 35-35
tests/test__parsing.py (1)
11-11:✅ Verification successful
Verify casting behavior for positional arguments.
The test now expects numeric strings to be cast to their respective types (
"2"→2,"3.5"→3.5), which is a significant behavior change. This automatic casting could break existing code that expects these values to remain as strings.Please verify that this casting behavior is intentional and won't break existing integrations:
🏁 Script executed:
#!/bin/bash # Description: Search for existing usages of parse_task_args to assess backwards compatibility impact # Expected: Find usages that might be affected by the string-to-number casting change echo "=== Searching for parse_task_args usage ===" rg -A 5 -B 5 "parse_task_args" --type py echo -e "\n=== Searching for potential string comparisons that might break ===" rg -A 3 -B 3 "args\[.*\].*==.*['\"][\d\.]+['\"]" --type pyLength of output: 4061
Casting behavior is isolated and intentional
parse_task_argsis only invoked inleverage/modules/run.py(and its own tests), so no other code paths will be affected.- A grep for comparisons against numeric strings (
'["]\d+(\.\d+)?["]') returned only unrelated test assertions (e.g., intests/test_modules/test_auth.py), so there’s no evidence of existing code relying on string‐typed numbers fromparse_task_args.- The updated tests in
tests/test__parsing.pycorrectly reflect the new casting behavior.Since the change is confined to the CLI argument parser and all existing call sites have been adjusted or are unaffected, this casting change appears safe. Please ensure any user documentation or examples are updated to note that numeric and boolean strings will now be cast automatically.
| [], | ||
| {"kwarg1": "/val/1", "kwarg2": "val2", "kwarg3": "val3"}, | ||
| {"kwarg1": True, "kwarg2": "val2", "kwarg3": 3}, | ||
| ), | ||
| ("arg1, arg2, kwarg1=/val/1,kwarg2 = val2", ["arg1", "arg2"], {"kwarg1": "/val/1", "kwarg2": "val2"}), # Both |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test coverage for mixed scenarios and edge cases.
The current tests cover basic scenarios well, but consider adding tests for these edge cases:
- Ambiguous strings: Version numbers like
"1.0", IDs like"123"that shouldn't be cast - Scientific notation:
"1e10","3.14e-2" - Leading zeros:
"007","0123" - Special float values:
"inf","nan"
+ # Edge cases for casting
+ ("1e10, inf, nan", [1e10, float('inf'), float('nan')], {}),
+ ("007, 0123", [7, 123], {}), # Leading zeros
+ ("kwarg1=1.0,kwarg2=0.0", [], {"kwarg1": 1.0, "kwarg2": 0.0}), # Ensure float casting📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ("arg1, arg2, kwarg1=/val/1,kwarg2 = val2", ["arg1", "arg2"], {"kwarg1": "/val/1", "kwarg2": "val2"}), # Both | |
| ("arg1, arg2, kwarg1=/val/1,kwarg2 = val2", ["arg1", "arg2"], {"kwarg1": "/val/1", "kwarg2": "val2"}), # Both | |
| # Edge cases for casting | |
| ("1e10, inf, nan", [1e10, float('inf'), float('nan')], {}), | |
| ("007, 0123", [7, 123], {}), # Leading zeros | |
| ("kwarg1=1.0,kwarg2=0.0", [], {"kwarg1": 1.0, "kwarg2": 0.0}), # Ensure float casting |
🤖 Prompt for AI Agents
In tests/test__parsing.py at line 17, the current test cases do not cover edge
cases involving ambiguous strings, scientific notation, leading zeros, and
special float values. Add new test cases that include inputs like "1.0", "123",
"1e10", "3.14e-2", "007", "0123", "inf", and "nan" to verify the parsing logic
correctly handles these scenarios without incorrect casting or errors.
| "kwarg1=true,kwarg2 = val2, kwarg3 = 3 ", | ||
| [], | ||
| {"kwarg1": "/val/1", "kwarg2": "val2", "kwarg3": "val3"}, | ||
| {"kwarg1": True, "kwarg2": "val2", "kwarg3": 3}, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify boolean casting implementation and edge cases.
The test expects "true" to be cast to True, but consider these edge cases that aren't covered:
- Case sensitivity: What about
"True","TRUE","False","false"? - Other boolean representations:
"yes","no","1","0"? - Invalid boolean strings:
"tru","fals"?
Consider adding test cases for boolean edge cases:
+ # Boolean edge cases
+ ("True, False, yes, no", [True, False, "yes", "no"], {}),
+ ("kwarg1=TRUE,kwarg2=false,kwarg3=1,kwarg4=0", [], {"kwarg1": True, "kwarg2": False, "kwarg3": 1, "kwarg4": 0}),Let's verify the actual boolean casting implementation:
🏁 Script executed:
#!/bin/bash
# Description: Find the cast_value function implementation to understand boolean casting logic
# Expected: Show how boolean strings are handled
echo "=== Finding cast_value function implementation ==="
ast-grep --pattern 'def cast_value($$$) {
$$$
}'
echo -e "\n=== Searching for boolean casting logic ==="
rg -A 10 -B 5 "bool|True|False" leverage/_casting.pyLength of output: 678
🏁 Script executed:
#!/bin/bash
# Display full cast_value implementation for comprehensive review
sed -n '1,200p' leverage/_casting.pyLength of output: 581
Add comprehensive boolean casting tests
The cast_value implementation lowercases inputs to handle "true"/"false" case-insensitively and then falls back to numeric casts (int or float), returning any other string as-is. To lock in this behavior and guard against regressions, please add tests covering:
- Case-insensitive booleans:
"True","TRUE","False","false" - Numeric strings → ints:
"1","0" - Other strings unchanged:
"yes","no", invalid boolean tokens like"tru","fals"
For example, in tests/test__parsing.py:
--- tests/test__parsing.py
+++ tests/test__parsing.py
@@ (existing tests)
# Boolean edge cases
- # (add your tests here)
+ ("True, FALSE, yes, no", [True, False, "yes", "no"], {}),
+ ("kwarg1=TRUE,kwarg2=false,kwarg3=1,kwarg4=0", [], {"kwarg1": True, "kwarg2": False, "kwarg3": 1, "kwarg4": 0}),This will confirm that:
"True"/"FALSE"→True/False"1"/"0"→1/0"yes"/"no"stay as strings- malformed tokens like
"tru"remain unchanged
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "kwarg1=true,kwarg2 = val2, kwarg3 = 3 ", | |
| [], | |
| {"kwarg1": "/val/1", "kwarg2": "val2", "kwarg3": "val3"}, | |
| {"kwarg1": True, "kwarg2": "val2", "kwarg3": 3}, | |
| # Boolean edge cases | |
| ("True, FALSE, yes, no", [True, False, "yes", "no"], {}), | |
| ("kwarg1=TRUE,kwarg2=false,kwarg3=1,kwarg4=0", [], {"kwarg1": True, "kwarg2": False, "kwarg3": 1, "kwarg4": 0}), |
🤖 Prompt for AI Agents
In tests/test__parsing.py around lines 13 to 15, the current test only checks
the boolean string "true" being cast to True but misses other important boolean
edge cases. Add test cases to cover case-insensitive boolean strings like
"True", "TRUE", "False", "false", numeric strings "1" and "0" which should cast
to integers, and other strings like "yes", "no", and malformed tokens like "tru"
and "fals" which should remain unchanged as strings. This will ensure the
cast_value function's boolean casting behavior is fully tested and guarded
against regressions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
leverage/_casting.py (1)
20-33: Consider the mixed parsing approach implications.The implementation uses YAML parsing only for booleans but falls back to direct int/float conversion for numeric types. While functional, this creates an inconsistency where some YAML-style inputs (like
"yes"/"no"for booleans) are parsed but others (like"1.5"vs"1.50") may not be handled consistently.Consider one of these approaches for consistency:
Option 1: Use YAML for all types (if you want full YAML compatibility):
def cast_value(value: str) -> Any: """Try to cast a string to bool, int or float using PyYAML.""" try: parsed = yaml.safe_load(value) - if isinstance(parsed, bool): - return parsed + if isinstance(parsed, (bool, int, float)): + return parsed except yaml.YAMLError: pass - - try: - return int(value) - except ValueError: - try: - return float(value) - except ValueError: - return value + return valueOption 2: Use direct parsing for consistency (if you want to avoid YAML complexity):
def cast_value(value: str) -> Any: """Try to cast a string to bool, int or float.""" - try: - parsed = yaml.safe_load(value) - if isinstance(parsed, bool): - return parsed - except yaml.YAMLError: - pass + # Handle common boolean representations + if value.lower() in ('true', 'false', 'yes', 'no', 'on', 'off'): + return value.lower() in ('true', 'yes', 'on') try: return int(value) except ValueError: try: return float(value) except ValueError: return valueThe current implementation works correctly but may surprise users when some YAML-style inputs work for booleans but not for other types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
leverage/_casting.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unit_tests (3.12)
- GitHub Check: unit_tests (3.11)
- GitHub Check: unit_tests (3.10)
- GitHub Check: unit_tests (3.9)
🔇 Additional comments (2)
leverage/_casting.py (2)
1-8: LGTM! Clean module structure.The module imports and structure are well-organized. Using
yaml.safe_loadis a secure choice for parsing.
10-19: Well-documented function signature.The function signature and docstring clearly explain the purpose and behavior. The type hints are appropriate for this use case.
|
@angelofenoglio @Franr @diego-ojeda-binbash I'm working on this issue here: https://chatgpt.com/s/cd_6841c45c38fc8191882b5efb7c4bb3b1 CC: @igomezraggio89 @leandro-binbash |
exequielrafaela
changed the title
Summary
cast_valueutility to detect bool/int/float.envvalues to their typesTesting
pytest -qhttps://chatgpt.com/codex/tasks/task_e_6841c005a738832fba0a8e5381fda35d
Summary by CodeRabbit
New Features
Bug Fixes
Tests