Skip to content

Return go types#26

Merged
nitram509 merged 2 commits intopbinitiative:mainfrom
trojanc:go-types
Mar 8, 2026
Merged

Return go types#26
nitram509 merged 2 commits intopbinitiative:mainfrom
trojanc:go-types

Conversation

@trojanc
Copy link
Contributor

@trojanc trojanc commented Oct 25, 2025

This PR attempts to address #3 and returns go types with EvalString and EvalStringWithScope

This MR also adds a small fix to have tck tests run

Assumptions made:

  • All lists with be of type []any
  • All maps will be of type map[string]any
  • All numbers will be returned as float64
  • Referencing structs with e.g get value will return the struct being referenced instead of a half baked map (mapstructure does unintended things....)

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @trojanc, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly refactors the FEEL expression evaluator to return native Go types from its primary evaluation functions. This change aims to enhance the usability and integration of the FEEL engine within Go applications by providing standard Go data structures and types directly, eliminating the need for manual unwrapping of custom FEEL types. The update also includes comprehensive test adjustments and a fix to ensure the TCK tests execute correctly, validating the new behavior.

Highlights

  • Native Go Type Returns: The core change involves modifying EvalString and EvalStringWithScope functions to return native Go types (e.g., float64, time.Time, nil) instead of custom FEEL-specific wrapper types (like *Number, *FEELDate).
  • Type Unwrapping Logic: A new unwrapFEELValue function has been introduced to recursively convert FEEL types and their container structures (lists, maps) into their corresponding native Go representations, ensuring consistency across the evaluation results.
  • Test Suite Updates: Extensive updates have been made to builtins_list_test.go, builtins_misc_test.go, and eval_test.go to reflect the new return types, including removing t.Skip calls and adjusting assertions to expect float64 for numbers and []any for lists.
  • TCK Test Fix: A small but crucial fix was implemented in tests/tck/tck_test.go to correctly handle test contexts, enabling the TCK (Technology Compatibility Kit) tests to run as intended.
  • Expected Value Parsing: The CreateExpected helper function in tests/feel_test_helper.go was enhanced to parse expected dateTime, date, decimal, double, and time values directly into standard Go types, improving test setup robustness.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request is a great step towards improving the library's usability by returning native Go types from EvalString and EvalStringWithScope. The changes are well-structured, with the core logic encapsulated in the new unwrapFEELValue function, and the test suite has been commendably updated to reflect these changes. However, I've identified a critical issue with how durations are handled and a high-severity issue regarding map key conversions, both of which could lead to silent data corruption. Addressing these points will be crucial for ensuring the reliability of the new API.

eval.go Outdated
Comment on lines +634 to +635
case *FEELDuration:
return val.Duration()
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

Returning val.Duration() which is a time.Duration is problematic for year-month durations. time.Duration is a nanosecond count and cannot represent variable units like years and months. The current implementation of FEELDuration.Duration() ignores Years and Months fields, leading to silent data loss and incorrect results when evaluating year-month durations (e.g., duration("P1Y") would incorrectly result in a 0 duration). Consider returning *FEELDuration itself or another struct that can preserve the year and month components to avoid this data corruption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really sure what to do about this....

Copy link
Member

Choose a reason for hiding this comment

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

worth another issue ... I will create one.

@nitram509 nitram509 merged commit 14d8930 into pbinitiative:main Mar 8, 2026
0 of 2 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.

2 participants