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

fix: Solves Issues with Set Function in Component Class and Enhance Input Handling #4258

Merged
merged 11 commits into from
Oct 24, 2024

Conversation

edwinjosechittilappilly
Copy link
Collaborator

This pull request addresses improvements in the Component class's input handling and introduces new unit tests to ensure robust functionality. The following changes have been made:

  1. Improved Input Processing:

    • Updated the _process_connection_or_parameters method in component.py to ensure that when a value is a list, it correctly processes only lists of Component instances. This enhancement prevents potential errors when other data types are passed.
  2. New Unit Tests:

    • Added test cases to verify the behavior of the set method in the Component class:
      • Mixed List Input Test: Checks that the set method can handle a list containing both a string and a component.
      • MessageTextInput List Test: Ensures that the set method can accept a list of MessageTextInput instances and that their values are set correctly.
      • Single Component Input Test: Validates that a single component can be set correctly.

Code Changes:

  • Component Logic:

    def _process_connection_or_parameters(self, key, value) -> None:
        # if value is a list of components, we need to process each component
        # Note this update makes sure it is a list of components and not of any other values
        if isinstance(value, list) and all(isinstance(item, Component) for item in value):
            for val in value:
                self._process_connection_or_parameter(key, val)
        else:
            self._process_connection_or_parameter(key, value)
  • New Tests:

    • Tests added in test_component_set_functionality.py to cover various input scenarios.

Notes:

  • This PR solves the inconsistency in input types when the component was run as a tool, enhancing the reliability and flexibility of the Component class by ensuring it can handle diverse input types effectively.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Oct 23, 2024
@edwinjosechittilappilly edwinjosechittilappilly changed the title fix: Enhance Component Input Handling and Add Unit Tests fix: Solves Issues with Set Function in Component Class and Enhance Input Handling Oct 23, 2024
@github-actions github-actions bot added bug Something isn't working and removed enhancement New feature or request labels Oct 23, 2024
Copy link
Contributor

@ogabrielluiz ogabrielluiz left a comment

Choose a reason for hiding this comment

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

LGTM

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 23, 2024
@@ -442,7 +442,8 @@ def _process_connection_or_parameter(self, key, value) -> None:

def _process_connection_or_parameters(self, key, value) -> None:
# if value is a list of components, we need to process each component
if isinstance(value, list):
# Note this update make sure it is a list of components and not of any other values
if isinstance(value, list) and all(isinstance(item, Component) for item in value):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like it's missing a case. We have:
a) IF value is a list AND they're all Components, process each item in the list separately.
b) ELSE, process the input as a single value.

What happens if it's a list, but they're not all Components?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it executes else and assigns the values as per key value pair

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if we will face that scenario if so we could add a case for it change the case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jordanrfrazier you were right in a way. I guess we should add multiple cases. Added a case just now to solve the pytest failure error.

@edwinjosechittilappilly
Copy link
Collaborator Author

In Pytest: test_starter_projects that tests
api/v1/starter-projects/
Am getting an Error: {"message":"["Something went wrong while serializing the response. Please share this error on our GitHub repository.", "Unable to serialize unknown type: <class 'method'>"]"}
Any suggestions: @ogabrielluiz @jordanrfrazier @italojohnny ?

Enhance component input handling and add unit tests for mixed input scenarios.
…tion_or_parameters

- Updated the _process_connection_or_parameters function to handle lists and dictionaries properly.
- Ensured that each element in a list is checked for serializability and converted to a string if necessary.
- Added logic to convert dictionaries to JSON strings, handling non-serializable contents by converting them to strings.
- This change prevents JSON serialization errors when processing component parameters.
updates _process_connection_or_parameters to handles situations where the list is not all component.

Also handles any serialisation isseus caused by _process_connection_or_parameters
@edwinjosechittilappilly
Copy link
Collaborator Author

@jordanrfrazier I have added a case where all the inputs are not components and updated the function accordingly.

Even though PR#4269 solves serialization error.
The function _ process_connection_or_parameters also caused some of those errors; hence, it was updated to solve the serialisation of values in _process_connection_or_parameters.

Tested with pytest and all cases passes.

@edwinjosechittilappilly edwinjosechittilappilly merged commit 78e6ee1 into main Oct 24, 2024
27 checks passed
@edwinjosechittilappilly edwinjosechittilappilly deleted the fix-component-set-functionality branch October 24, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants