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 optional generate field to input_schema #198

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

shekharnwagh
Copy link
Contributor

@shekharnwagh shekharnwagh commented Nov 14, 2024

Description

This PR introduces a new optional 'generate' function field to the query input schema. This allows for dynamic query input generation which would be required for modifying the query input. One of the use cases for this is the PR #191 where we need to append the ".md" suffix to the github file path if not provided.

Changes:

  • Enhanced Query Input Handling:

    • Added support for generate functions in query input schemas to facilitate dynamic query input generation. This function receives the input values array as input and returns a new value which will be used as the value for the input field.
    • Implemented transform_query_input_with_generators method to apply these transformations before executing queries.
    • Modified get_query_input method to incorporate query input transformations after applying overrides.
  • Updated Query Execution Logic:

    • Updated execute_query method to use the new get_query_input method, ensuring that query inputs are correctly transformed and overridden before execution.
  • Unit Tests Added

    • Added unit tests for BlockBindings class in BlockBindingsTest.php covering:
      • Execution of queries with no configuration.
      • Execution of queries returning results.
      • Application of query input overrides.
      • Query input transformations with single and multiple inputs.
      • Combined transformations and overrides.

@shekharnwagh shekharnwagh self-assigned this Nov 14, 2024
@@ -37,6 +37,10 @@ class HttpQueryContext implements QueryContextInterface, HttpQueryContextInterfa
'type' => 'array',
'required' => false,
],
'generate' => [
Copy link
Contributor Author

@shekharnwagh shekharnwagh Nov 14, 2024

Choose a reason for hiding this comment

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

Thought about naming this as transform or filter, but ended up naming it as generate to match with name for similar field in the output_schema -

'generate' => [
'type' => 'function',
'required' => false,
],

Wouldn't mind using another name if there is an good reason to do it.

Comment on lines +67 to +69
/**
* @runInSeparateProcess
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to add the @runInSeparateProcess decorator to prevent static method mocks for the ConfigStore class from causing side effects in other tests. Although using dependency injection instead of static methods could be a better approach, it would require significant changes due to the heavy use of static methods in this codebase. Open to suggestions for better ways to handle static methods in tests.

@shekharnwagh
Copy link
Contributor Author

Used GitHub Copilot to generate the PR Description - Everything except the first paragraph. Let me what do you think of them.

@shekharnwagh shekharnwagh marked this pull request as ready for review November 14, 2024 21:18
Copy link
Contributor

@maxschmeling maxschmeling left a comment

Choose a reason for hiding this comment

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

I debated about the generate name in my head a pretty good amount. I think a name like transform would make more sense here because, the way it's written now, you can't really "generate" new stuff... you can just return a different value that what was there. I like the consistency with the output mappings, but there you're actually generating an entirely new field that otherwise wouldn't be in the config.

Though I get that, depending on the use case and how you implement it, the difference might not actually be that big...

So I don't care too much either way, but I would vote for changing it. But I'll leave it up to you.

Rename 'generate' to 'transform' in query input schema and related methods to better reflect
their purpose of transforming input data. This change improves code clarity by using more
accurate terminology for data transformation operations.
@shekharnwagh
Copy link
Contributor Author

I think a name like transform would make more sense here because, the way it's written now, you can't really "generate" new stuff...

@maxschmeling This makes sense and when writing it transform felt more natural and better explained what the function was doing. Updated the name for the function.

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