Skip to content

Conversation

@yannrichet-asnr
Copy link
Member

No description provided.

Copilot AI review requested due to automatic review settings January 24, 2026 21:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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
Comment on lines 265 to 358
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", "@")
)
)
)
)
)
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines 372 to 382
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
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
fz/core.py Outdated
Comment on lines 1044 to 1049
# 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:
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
# 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):

Copilot uses AI. Check for mistakes.
dedented_code = textwrap.dedent(full_code)

try:
exec(dedented_code, env)
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines +536 to +538
# Extract context lines from model if available
context_lines = []

Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

Variable context_lines is not used.

Suggested change
# Extract context lines from model if available
context_lines = []

Copilot uses AI. Check for mistakes.

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
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

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

Import of 'get_config' is not used.

Suggested change
from .config import get_config, get_interpreter
from .config import get_interpreter

Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@yannrichet yannrichet marked this pull request as draft January 25, 2026 09:37
yannrichet and others added 8 commits January 25, 2026 10:37
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>
@yannrichet yannrichet marked this pull request as ready for review January 25, 2026 17:13
@yannrichet yannrichet merged commit 0daf661 into main Jan 25, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants