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

[WIP] Add Intrinsic Functions #84

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

agrare
Copy link
Member

@agrare agrare commented Aug 8, 2023

Add the basics for Intrinsic Function support and implement StringToJson

Closes #64

@agrare agrare requested a review from Fryguy as a code owner August 8, 2023 18:01
@agrare agrare added the wip label Aug 8, 2023
@agrare agrare requested a review from kbrock August 8, 2023 18:01
@agrare agrare changed the title Add Intrinsic Functions [WIP] Add Intrinsic Functions Aug 8, 2023
def value(context, inputs)
arg = args.first
arg =
if arg.kind_of?(Floe::Workflow::Path) || arg.kind_of?(Floe::Workflow::IntrinsicFunction)
Copy link
Member

Choose a reason for hiding this comment

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

Would it be cleaner to say arg.respond_to?(:value)? Then if we have future evauable things they would just work. That makes me wonder if it might be nice to have an Evaulable mixin, but I'm not sure what exactly we'd put into it. I am seeing a lot of the same pattern of if arg.start_with?("$.") all over the place, which gets me wondering if there's way to DRY that up.

@agrare agrare force-pushed the add_intrinsic_functions branch 2 times, most recently from 195a26f to 875fd61 Compare August 9, 2023 14:29
INTRINSIC_FUNCTION_REGEX = /^(?<module>\w+)\.(?<function>\w+)\((?<args>.*)\)$/.freeze

class << self
def klass(payload)
Copy link
Member

Choose a reason for hiding this comment

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

Minor, but I think it's clearer to describe the name with something like

Suggested change
def klass(payload)
def detect_class(payload)

@Fryguy
Copy link
Member

Fryguy commented Aug 9, 2023

I realize this is WIP, so I'll let it bake more - this looks really great!

@agrare agrare force-pushed the add_intrinsic_functions branch 2 times, most recently from 7b6146f to 32de795 Compare August 9, 2023 14:57

begin
function_module = Floe::Workflow::IntrinsicFunctions.const_get(function_module_name)
function_module.const_get(function_name)
Copy link
Member

@Fryguy Fryguy Aug 9, 2023

Choose a reason for hiding this comment

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

I have one concern here. I was looking through the list of Intrinsic Functions and there is one called States.Array. This would imply we have a Floe::Workflow::IntrinsicFunctions::States::Array class, and that Array shadowing kind of scares me (regardless of const_get or literal). I'm wondering if instead of a full namespacing, we instead just smash the module/function together so you'd get something more like: Floe::Workflow::IntrinsicFunctions::StatesArray. Similarly, there is a States::Hash class. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm we could have States be a class with methods def StringToJson and def Array rather than having classes for each intrinsic function. I like having these separated because if/when we have our own ManageIQ intrinsic functions they will be nicely separated.

Copy link
Member

Choose a reason for hiding this comment

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

maybe? methods named with capitals will surely get confusing. If they are class methods then, technicall :: will call those too, which works, but would still be weird. Reminds me of the Vmdb::BUILD method we have.

Copy link
Member

Choose a reason for hiding this comment

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

I like going the send route, aka: def StringToJson (or def intrinsicStringToJson if we need to start with a lowercase). Alternatively a case statement may not be the most ruby thing, but that tends to be the defacto implementation for this type of construct across all programming languages (both target language and complier/interpreter language).

All intrinsics are in the same namespace: States.*. I think hardcoding this rather than a generic "look for a period" works best, and we can hardcode any additional namespaces in the future. Lets also avoid regular expressions and use indexOf(")") kind of stuff. It is way too hard to deal with properly nesting parens and regular expressions. Though this implementation may be ok for a first pass.

@agrare agrare force-pushed the add_intrinsic_functions branch 2 times, most recently from e042ce0 to 5173bbe Compare August 9, 2023 18:41
@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2023

Checked commits agrare/floe@fad9dbc~...3db55e7 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
6 files checked, 0 offenses detected
Everything looks fine. 🍰

@miq-bot miq-bot added the stale label Dec 11, 2023
@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2023

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@agrare agrare removed the stale label Dec 11, 2023
@miq-bot miq-bot added the stale label Mar 18, 2024
@miq-bot
Copy link
Member

miq-bot commented Mar 18, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support intrinsic functions
4 participants