Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds support for the original Java Funz variable and formula syntax to maintain compatibility with existing Java Funz workflows. The PR introduces several significant features:
Changes:
- Adds support for
()delimiters for variables (e.g.,$(var)) alongside the existing{}syntax (e.g.,${var}) - Implements static object definitions for constants and functions using
#@:prefix syntax - Adds formula evaluation with format specifiers (e.g.,
@{expr | 0.00}) - Introduces multiple model key aliases for backward compatibility (e.g.,
var_prefix/varprefix,formula_prefix/formulaprefix, etc.) - Modifies
fzi()to return a comprehensive dictionary including variables, formulas, and static objects with their values/expressions
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_static_objects.py | Comprehensive tests for static object definitions (constants and functions) with various interpreters |
| tests/test_no_variables.py | Minor variable naming changes (variables→result) for clarity |
| tests/test_model_key_aliases.py | Tests for all model key aliases (var_prefix, formula_prefix, comment) |
| tests/test_java_funz_compatibility.py | Tests verifying Java Funz syntax compatibility |
| tests/test_fzi_formulas.py | Tests for formula parsing and evaluation with default values |
| tests/test_cli_commands.py | Minor comment addition clarifying JSON output structure |
| fz/interpreter.py | Core implementation of parsing logic, formula evaluation, and static object handling |
| fz/helpers.py | Updates to use new model key aliases with backward compatibility |
| fz/core.py | Major changes to fzi() function and addition of helper functions for model key resolution |
| examples/java_funz_syntax_example.py | Example demonstrating Java Funz syntax usage |
| examples/fzi_static_objects_example.py | Example showing static object definitions |
| examples/fzi_formulas_example.py | Example demonstrating formula parsing and evaluation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fz/core.py
Outdated
| def _get_comment_char(model: Dict) -> str: | ||
| """ | ||
| Get comment character from model with support for multiple aliases | ||
|
|
||
| Supported keys (in order of precedence): | ||
| - commentline | ||
| - comment_line | ||
| - comment_char | ||
| - commentchar | ||
| - comment | ||
|
|
||
| Args: | ||
| model: Model definition dict | ||
|
|
||
| Returns: | ||
| Comment character (default "#") | ||
| """ | ||
| return model.get( | ||
| "commentline", | ||
| model.get( | ||
| "comment_line", | ||
| model.get( | ||
| "comment_char", | ||
| model.get( | ||
| "commentchar", | ||
| model.get("comment", "#") | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def _get_var_prefix(model: Dict) -> str: | ||
| """ | ||
| Get variable prefix from model with support for multiple aliases | ||
|
|
||
| Supported keys (in order of precedence): | ||
| - var_prefix | ||
| - varprefix | ||
| - var_char | ||
| - varchar | ||
|
|
||
| Args: | ||
| model: Model definition dict | ||
|
|
||
| Returns: | ||
| Variable prefix (default "$") | ||
| """ | ||
| return model.get( | ||
| "var_prefix", | ||
| model.get( | ||
| "varprefix", | ||
| model.get( | ||
| "var_char", | ||
| model.get("varchar", "$") | ||
| ) | ||
| ) | ||
| ) | ||
|
|
||
|
|
||
| def _get_formula_prefix(model: Dict) -> str: | ||
| """ | ||
| Get formula prefix from model with support for multiple aliases | ||
|
|
||
| Supported keys (in order of precedence): | ||
| - formula_prefix | ||
| - formulaprefix | ||
| - form_prefix | ||
| - formprefix | ||
| - formula_char | ||
| - form_char | ||
|
|
||
| Args: | ||
| model: Model definition dict | ||
|
|
||
| Returns: | ||
| Formula prefix (default "@") | ||
| """ | ||
| return model.get( | ||
| "formula_prefix", | ||
| model.get( | ||
| "formulaprefix", | ||
| model.get( | ||
| "form_prefix", | ||
| model.get( | ||
| "formprefix", | ||
| model.get( | ||
| "formula_char", | ||
| model.get("form_char", "@") | ||
| ) | ||
| ) | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
These three helper functions (_get_comment_char, _get_var_prefix, _get_formula_prefix) are duplicated from fz/interpreter.py. This violates the DRY (Don't Repeat Yourself) principle and creates maintenance issues. If the logic needs to change, it must be updated in both places. Consider importing these functions from interpreter.py instead of redefining them here.
fz/interpreter.py
Outdated
| if '=' in line and not line.startswith('def ') and '<-' not in line: | ||
| parts = line.split('=', 1) | ||
| if len(parts) == 2: | ||
| name = parts[0].strip() | ||
| # Remove any type hints | ||
| if ':' in name: | ||
| name = name.split(':')[0].strip() | ||
| value = parts[1].strip() | ||
| expressions[name] = value | ||
| i += 1 | ||
| continue |
There was a problem hiding this comment.
This check for Python assignment may incorrectly match comparison operators. For example, a line like "if x == value:" contains '=' but should not be treated as an assignment. The check should use a more robust pattern, such as checking for '==' or other comparison operators first, or using a proper AST parser. Consider using regex to match assignment patterns more precisely: r'^\s*([a-zA-Z_][a-zA-Z0-9_])\s=\s*(?!=)' to avoid matching '==', '<=', '>=', '!=', etc.
fz/core.py
Outdated
| # Try numeric conversion | ||
| if '.' in default_value or 'e' in default_value.lower(): | ||
| variable_defaults[var_name] = float(default_value) | ||
| else: | ||
| variable_defaults[var_name] = int(default_value) | ||
| except ValueError: |
There was a problem hiding this comment.
The numeric conversion logic may fail for certain valid numeric formats. For example, hexadecimal numbers (0x1F), octal numbers (0o77), binary numbers (0b1010), or numbers with underscores (1_000_000) are valid in Python but won't be handled correctly here. Consider using ast.literal_eval() for more robust parsing, which can handle various Python literal formats safely.
| # Try numeric conversion | |
| if '.' in default_value or 'e' in default_value.lower(): | |
| variable_defaults[var_name] = float(default_value) | |
| else: | |
| variable_defaults[var_name] = int(default_value) | |
| except ValueError: | |
| # Try numeric conversion using ast.literal_eval to support | |
| # various Python numeric literal formats (hex, octal, binary, | |
| # underscores, etc.). | |
| evaluated = ast.literal_eval(default_value) | |
| if isinstance(evaluated, (int, float)): | |
| variable_defaults[var_name] = evaluated | |
| else: | |
| # Not a numeric literal; fall through to string handling | |
| raise ValueError | |
| except (ValueError, SyntaxError): |
| dedented_code = textwrap.dedent(full_code) | ||
|
|
||
| try: | ||
| exec(dedented_code, env) |
There was a problem hiding this comment.
Using exec() to evaluate arbitrary user code from input files poses a significant security risk. If an attacker can control the input file content, they can execute arbitrary Python code on the system. While this may be acceptable for trusted input files in certain use cases, consider adding a warning in the documentation about this security implication, or implementing a sandboxed execution environment. At minimum, document that input files should only come from trusted sources.
| # Extract context lines from model if available | ||
| context_lines = [] | ||
|
|
There was a problem hiding this comment.
Variable context_lines is not used.
| # Extract context lines from model if available | |
| context_lines = [] |
|
|
||
| from .logging import log_error, log_warning, log_info, log_debug, log_progress | ||
| from .config import get_config | ||
| from .config import get_config, get_interpreter |
There was a problem hiding this comment.
Import of 'get_config' is not used.
| from .config import get_config, get_interpreter | |
| from .config import get_interpreter |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
No description provided.