Skip to content

Enhancement | Issue #21 | OPT-2 | Implement typed argument casting #307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Enhancement | Issue #21 | OPT-2 | Implement typed argument casting #307

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 53 additions & 0 deletions leverage/_casting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
"""
Value casting utilities.
"""

from typing import Any

import yaml


def as_bool(value: str) -> Any:
"""Return the boolean representation of ``value`` if possible."""
try:
parsed = yaml.safe_load(value)
if isinstance(parsed, bool):
return parsed
except yaml.YAMLError:
pass
return value
Comment on lines +10 to +18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security concern: yaml.safe_load can parse more than just booleans.

Using yaml.safe_load for boolean parsing is risky as it can interpret various YAML constructs beyond simple boolean values. This could lead to unexpected behavior or security issues.

Consider using a more explicit boolean parsing approach:

-def as_bool(value: str) -> Any:
-    """Return the boolean representation of ``value`` if possible."""
-    try:
-        parsed = yaml.safe_load(value)
-        if isinstance(parsed, bool):
-            return parsed
-    except yaml.YAMLError:
-        pass
-    return value
+def as_bool(value: str) -> Any:
+    """Return the boolean representation of ``value`` if possible."""
+    lower_value = value.lower().strip()
+    if lower_value in ('true', 'yes', '1', 'on'):
+        return True
+    elif lower_value in ('false', 'no', '0', 'off'):
+        return False
+    return value
📝 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.

Suggested change
def as_bool(value: str) -> Any:
"""Return the boolean representation of ``value`` if possible."""
try:
parsed = yaml.safe_load(value)
if isinstance(parsed, bool):
return parsed
except yaml.YAMLError:
pass
return value
def as_bool(value: str) -> Any:
"""Return the boolean representation of ``value`` if possible."""
lower_value = value.lower().strip()
if lower_value in ('true', 'yes', '1', 'on'):
return True
elif lower_value in ('false', 'no', '0', 'off'):
return False
return value
🤖 Prompt for AI Agents 
In leverage/_casting.py around lines 10 to 18, the use of yaml.safe_load to
parse booleans is insecure because it can interpret complex YAML structures
beyond simple booleans. Replace yaml.safe_load with a more explicit boolean
parsing method that only recognizes specific string values like "true", "false",
"yes", "no" (case-insensitive) and returns the corresponding boolean, otherwise
return the original value.



def as_int(value: str) -> Any:
"""Return the integer representation of ``value`` if possible."""
try:
return int(value)
except ValueError:
return value


def as_float(value: str) -> Any:
"""Return the float representation of ``value`` if possible."""
try:
return float(value)
except ValueError:
return value


def cast_value(value: str) -> Any:
"""Try to cast ``value`` to bool, int or float using the helper functions
:func:`as_bool`, :func:`as_int` and :func:`as_float`.

Args:
value (str): Value to cast.

Returns:
Any: The value converted to its apparent type or the original string.
"""
value = as_bool(value)
if isinstance(value, str):
value = as_int(value)
if isinstance(value, str):
value = as_float(value)

return value
Comment on lines +37 to +53
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add input validation and improve type safety.

The function accepts str type but doesn't validate the input. Also, the casting order could cause issues where numeric strings like "123.0" are cast as integers instead of floats.

 def cast_value(value: str) -> Any:
-    """Try to cast ``value`` to bool, int or float using the helper functions
-    :func:`as_bool`, :func:`as_int` and :func:`as_float`.
-
-    Args:
-        value (str): Value to cast.
-
-    Returns:
-        Any: The value converted to its apparent type or the original string.
-    """
+    """Try to cast ``value`` to bool, int or float using the helper functions
+    :func:`as_bool`, :func:`as_int` and :func:`as_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 = as_bool(value)
     if isinstance(value, str):
+        # Try float before int to handle decimal numbers correctly
+        temp_float = as_float(value)
+        if not isinstance(temp_float, str):
+            # Check if it's actually an integer value
+            if temp_float.is_integer():
+                return int(temp_float)
+            return temp_float
         value = as_int(value)
-    if isinstance(value, str):
-        value = as_float(value)
 
     return value
📝 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.

Suggested change
def cast_value(value: str) -> Any:
"""Try to cast ``value`` to bool, int or float using the helper functions
:func:`as_bool`, :func:`as_int` and :func:`as_float`.
Args:
value (str): Value to cast.
Returns:
Any: The value converted to its apparent type or the original string.
"""
value = as_bool(value)
if isinstance(value, str):
value = as_int(value)
if isinstance(value, str):
value = as_float(value)
return value
def cast_value(value: str) -> Any:
"""Try to cast ``value`` to bool, int or float using the helper functions
:func:`as_bool`, :func:`as_int` and :func:`as_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 = as_bool(value)
if isinstance(value, str):
# Try float before int to handle decimal numbers correctly
temp_float = as_float(value)
if not isinstance(temp_float, str):
# Check if it's actually an integer value
if temp_float.is_integer():
return int(temp_float)
return temp_float
value = as_int(value)
return value
🤖 Prompt for AI Agents 
In leverage/_casting.py around lines 37 to 53, add input validation to ensure
the input is a string before processing to improve type safety. Adjust the
casting order to first attempt casting to float, then int, and finally bool, to
correctly handle numeric strings like "123.0" without prematurely casting them
as integers. Update the function to raise a TypeError if the input is not a
string.

6 changes: 4 additions & 2 deletions leverage/_parsing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
Command line arguments and tasks arguments parsing utilities.
"""

from leverage._casting import cast_value


class InvalidArgumentOrderError(RuntimeError):
pass
Expand Down Expand Up @@ -40,13 +42,13 @@ def parse_task_args(arguments):
f"Positional argument `{argument}` from task `{{task}}` cannot follow a keyword argument."
)

args.append(argument.strip())
args.append(cast_value(argument.strip()))

else:
key, value = [part.strip() for part in argument.split("=")]
if key in kwargs:
raise DuplicateKeywordArgumentError(f"Duplicated keyword argument `{key}` in task `{{task}}`.")

kwargs[key] = value
kwargs[key] = cast_value(value)

return args, kwargs
4 changes: 3 additions & 1 deletion leverage/conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@

from yaenv.core import Env

from leverage._casting import cast_value

from leverage import logger
from leverage.path import get_root_path
from leverage.path import get_working_path
Expand Down Expand Up @@ -56,6 +58,6 @@ def load(config_filename=ENV_CONFIG_FILE):
config_file = Env(config_file_path)

for key, val in config_file:
config_dict[key] = val
config_dict[key] = cast_value(val)

return config_dict
23 changes: 19 additions & 4 deletions tests/test__parsing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,35 @@
@pytest.mark.parametrize(
"arguments, expected_args, expected_kwargs",
[
("arg1, arg2, arg3 ", ["arg1", "arg2", "arg3"], {}), # All positional arguments
("arg1, 2, 3.5 ", ["arg1", 2, 3.5], {}), # Cast positional arguments
( # All keyworded arguments
"kwarg1=/val/1,kwarg2 = val2, kwarg3 = val3 ",
"kwarg1=true,kwarg2 = val2, kwarg3 = 3 ",
[],
{"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
# 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
# Boolean edge cases
("True, FALSE, yes, no", [True, False, True, False], {}),
(
"kwarg1=TRUE,kwarg2=false,kwarg3=1,kwarg4=0",
[],
{"kwarg1": True, "kwarg2": False, "kwarg3": 1, "kwarg4": 0},
),
(None, [], {}), # No arguments
],
)
def test__parse_args(arguments, expected_args, expected_kwargs):
args, kwargs = parse_task_args(arguments=arguments)
for received, expected in zip(args, expected_args):
if isinstance(expected, float) and expected != expected: # NaN check
assert received != received
else:
assert received == expected

assert args == expected_args
assert kwargs == expected_kwargs


Expand Down
38 changes: 38 additions & 0 deletions tests/test_casting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import math

import pytest

from leverage._casting import as_bool, as_int, as_float, cast_value


@pytest.mark.parametrize(
"value, expected",
[
("true", True),
("False", False),
("1", 1),
("-2", -2),
("3.14", 3.14),
("1e3", 1000.0),
("inf", float("inf")),
("nan", float("nan")),
("007", 7),
("0123", 123),
("foo", "foo"),
],
)
def test_cast_value(value, expected):
result = cast_value(value)
if isinstance(expected, float) and math.isnan(expected):
assert math.isnan(result)
else:
assert result == expected


def test_helper_functions():
assert as_bool("true") is True
assert as_bool("no") is False
assert as_int("42") == 42
assert as_int("bar") == "bar"
assert as_float("3.14") == 3.14
assert as_float("not") == "not"
4 changes: 2 additions & 2 deletions tests/test_conf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@
True,
{
"PROJECT": "foobar",
"MFA_ENABLED": "true",
"MFA_ENABLED": True,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify boolean casting behavior aligns with expectations.

The test now expects True instead of "true" for MFA_ENABLED. This change aligns with the new casting functionality, but ensure this behavioral change is intentional and won't break downstream code that expects string values.

🏁 Script executed:

#!/bin/bash
# Description: Search for usages of MFA_ENABLED to verify impact of type change

# Search for MFA_ENABLED usage patterns
rg -A 3 -B 3 "MFA_ENABLED"

Length of output: 2238

Ensure consistent boolean conversion for MFA_ENABLED across modules.

Tests in tests/test_conf.py now expect native booleans, but downstream code still treats MFA_ENABLED as a string:

  • tests/test_modules/test_credentials.py fixture uses "MFA_ENABLED": "true" instead of True.
  • leverage/container.py does self.env_conf.get("MFA_ENABLED", "false") == "true" and has a # TODO: Convert values to bool upon loading.
  • leverage/modules/credentials.py assigns config_values["mfa_enabled"] directly from the raw string returned by env_config.get("MFA_ENABLED", "false").

Please update these places to use the new casting logic (returning booleans) and adjust the corresponding tests to expect True/False.

🤖 Prompt for AI Agents 
In tests/test_conf.py at line 33, the MFA_ENABLED value is changed to a boolean
True, but related code in tests/test_modules/test_credentials.py,
leverage/container.py, and leverage/modules/credentials.py still treat
MFA_ENABLED as a string. Update these files to consistently use boolean values
for MFA_ENABLED by applying the new casting logic that converts environment
variables to booleans, and modify their tests to expect True/False instead of
"true"/"false" strings.

"ENTRYPOINT": "/bin/run",
"DEBUG": "true",
"DEBUG": True,
"CONFIG_PATH": "/home/user/.config/",
},
),
Expand Down
Loading