Skip to content
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.

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

Add FactValue type #133

Merged
merged 9 commits into from
Nov 11, 2022
Merged

Add FactValue type #133

merged 9 commits into from
Nov 11, 2022

Conversation

fabriziosestito
Copy link
Member

@fabriziosestito fabriziosestito commented Nov 10, 2022

Adds FactValue variant type and the possibility to return lists or maps from the gatherer.
Using variant types with sealed interfaces is not the most idiomatic thing in golang, but the structpb package from google uses the same trick to define the google protobuf value, and it saves us some runtime errors if the users returns types that we do not handle as interface{}.
The conversion to a protobuf message is straight forward by just using the AsInterface() method.

Existings gatehrers and plugins are refactored to comply with the new return type.

Also, I've added float and bool type and an utility function to parse a string to a number or a bool.

@fabriziosestito fabriziosestito changed the title wip Add FactValue type Nov 10, 2022
@fabriziosestito fabriziosestito marked this pull request as ready for review November 11, 2022 12:23
@fabriziosestito fabriziosestito requested review from arbulu89, CDimonaco and rtorrero and removed request for arbulu89, CDimonaco and rtorrero November 11, 2022 12:23
Copy link
Contributor

@arbulu89 arbulu89 left a comment

Choose a reason for hiding this comment

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

Brilliant!
Love the change!

internal/factsengine/entities/fact_value.go Outdated Show resolved Hide resolved
internal/factsengine/mapper_test.go Show resolved Hide resolved
Copy link
Member

@CDimonaco CDimonaco left a comment

Choose a reason for hiding this comment

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

LGTM, great work!

@fabriziosestito fabriziosestito merged commit ca624a5 into main Nov 11, 2022
@fabriziosestito fabriziosestito deleted the add_fact_value_sum_type branch November 11, 2022 14:51
@arbulu89 arbulu89 added the enhancement New feature or request label Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

3 participants