Richer support for python syntax#4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the formula parser/evaluator to better match Python-like syntax by adding expr.property access (e.g., math.pi, ('a','b').index) and enabling keyword arguments for selected built-ins (e.g., min(..., key=...), round(x=..., base=...)). This supports the target expression min(extruderValues('adhesion_type'), key=('raft', 'brim', 'skirt', 'none').index).
Changes:
- Introduces
PropertyAccessExprplus parser grammar support for chained property access on variables and primary expressions. - Adds keyword-argument parsing and evaluation via a new
eval::Value::rich_fn_t(function + signature), and wires signatures intoint,round,min, andmath.log. - Updates the standard environment to expose
mathas an object map instead ofmath.*flat variables; adds/updates tests accordingly.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/parser.cpp | Updates existing math-related AST expectations and adds tests for property access, tuple/list methods, and keyword args/key usage. |
| src/eval.cpp | Extends Value formatting/equality/truthiness/emscripten conversion for rich_fn_t and object maps. |
| src/env/round.cpp | Refactors round into a signature-bearing functor and keeps legacy fn_t wrapper. |
| src/env/min.cpp | Adds key support (callable second argument) and signature-bearing functor wrapper. |
| src/env/math_log.cpp | Refactors math.log into a signature-bearing functor and keeps legacy fn_t wrapper. |
| src/env/map.cpp | Allows map() to accept either fn_t or rich_fn_t callables. |
| src/env/int_fn.cpp | Refactors int into a signature-bearing functor and keeps legacy fn_t wrapper. |
| src/env/env.cpp | Switches math into a property-map object; registers int/min/round/math.log as rich_fn_t in std env. |
| src/ast/property_access_expr.cpp | Implements evaluation for property maps and built-in methods like join and index. |
| src/ast/fn_application_expr.cpp | Implements keyword-arg binding and invocation for rich_fn_t functions. |
| include/cura-formulae-engine/parser/variable_or_fn_application_grammar_or_array_indexing_grammar.h | Adds keyword-arg parsing, property-access parsing, and supports applying these to primary expressions. |
| include/cura-formulae-engine/parser/variable_grammar.h | Disallows . inside identifiers to enable expr.property parsing. |
| include/cura-formulae-engine/parser/math_expr_grammar.h | Simplifies atom parsing by delegating primaries to the variable/fn/index/property grammar. |
| include/cura-formulae-engine/formula.md | Updates grammar snippet to mention keyword-arg syntax. |
| include/cura-formulae-engine/eval.h | Adds rich_fn_t and object-map support to Value. |
| include/cura-formulae-engine/env/round.h | Declares RoundFunction and exported instance for signature support. |
| include/cura-formulae-engine/env/min.h | Declares MinFunction and exported instance for signature support. |
| include/cura-formulae-engine/env/math_log.h | Declares MathLogFunction and exported instance for signature support. |
| include/cura-formulae-engine/env/int_fn.h | Declares IntFunction and exported instance for signature support. |
| include/cura-formulae-engine/ast/property_access_expr.h | Declares the new PropertyAccessExpr AST node. |
| include/cura-formulae-engine/ast/fn_application_expr.h | Extends AST node with keyword-arg storage/comparison. |
| CMakeLists.txt | Enables CTest discovery and adds the new AST source file to the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Ultimaker/CuraFormulaeEngine/sessions/bb29b144-a878-4363-8aec-f3d1835aad79 Co-authored-by: casperlamboo <6638028+casperlamboo@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Ultimaker/CuraFormulaeEngine/sessions/bb29b144-a878-4363-8aec-f3d1835aad79 Co-authored-by: casperlamboo <6638028+casperlamboo@users.noreply.github.com>
Erwan MATHIEU (wawanbreton)
left a comment
There was a problem hiding this comment.
Looks good ! Only a few remarks/suggestion. Of course I don't understand 100% of it, but the unit tests should be good enough to check the behavior 🙂
| std::vector<KeywordArg> kwargs; | ||
|
|
||
| FnApplicationExpr(ExprPtr fn, std::vector<ExprPtr> args) | ||
| FnApplicationExpr(ExprPtr fn, std::vector<ExprPtr> args, std::vector<KeywordArg> kwargs = {}) |
There was a problem hiding this comment.
For the move to be really effective, isn't it necessary to declare the arguments as move-able ? Otherwise there will be a first copy, then the copy will be moved, right ? 🤔
If this is correct, then the same remark applies to all the constructors.
| FnApplicationExpr(ExprPtr fn, std::vector<ExprPtr> args, std::vector<KeywordArg> kwargs = {}) | |
| FnApplicationExpr(ExprPtr fn, std::vector<ExprPtr> && args, std::vector<KeywordArg> && kwargs = {}) |
|
|
||
| [[nodiscard]] std::optional<std::vector<std::string>> getSignature() const noexcept | ||
| { | ||
| if (signature.empty()) |
There was a problem hiding this comment.
Can't the signature be empty if it has no arguments ?
| { | ||
| if (slots[i].has_value()) | ||
| { | ||
| return std::nullopt; |
There was a problem hiding this comment.
Maybe add a spdlog::error since InvalidNumberOfArguments is not super explicit ?
| if (map_lhs.size() != map_rhs.size()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| for (const auto& [key, val_lhs] : map_lhs) | ||
| { | ||
| const auto it = map_rhs.find(key); | ||
| if (it == map_rhs.end() || val_lhs != it->second) | ||
| { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; |
There was a problem hiding this comment.
Can you use the == operator of the map here ? It seems to behave the same 🤔
| if (map_lhs.size() != map_rhs.size()) | |
| { | |
| return false; | |
| } | |
| for (const auto& [key, val_lhs] : map_lhs) | |
| { | |
| const auto it = map_rhs.find(key); | |
| if (it == map_rhs.end() || val_lhs != it->second) | |
| { | |
| return false; | |
| } | |
| } | |
| return true; | |
| return map_lhs == map_rhs; |
|
Oh also I think it would make sense to bump the version to 1.2, right ? |
Main objective of this PR is to be able to parse the following value
Added support for the following syntax
property.indexmin(..., key=...)NP-1167, NP-1333