Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion apps/application/flow/compare/contain_compare.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ def support(self, node_id, fields: List[str], source_value, compare, target_valu
def compare(self, source_value, compare, target_value):
if isinstance(source_value, str):
return str(target_value) in source_value
return any([str(item) == str(target_value) for item in source_value])
elif isinstance(source_value, list):
return any([str(item) == str(target_value) for item in source_value])
else:
return str(target_value) in str(source_value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code looks good for most cases, but there are a couple of minor improvements that could be made:

  1. Consistency in Error Handling: The current function handles lists differently from strings and other types. It's slightly more consistent to handle all non-string types uniformly.

  2. Type Checking: Ensure that source_value is not empty when checking against it directly with if isinstance(source_value, str).

  3. Code Readability: Consider adding docstrings to explain each method, though they already exist.

Here is the improved version:

def compare(self, source_value, compare, target_value):
    if compare == "equal":
        if not source_value or not target_value:
            return False
        if isinstance(source_value, str):
            return str(target_value) == str(source_value)
        elif isinstance(source_value, list):
            return any(str(item) == str(target_value) for item in source_value)
        else:
            return str(target_value) == str(source_value)

This ensures better clarity and consistency across different input types. If you only want this behavior for equality checks (compare = "equal"), add logic similar for other comparison operators (e.g., "not_equal", "less_than", etc.) as needed.

5 changes: 4 additions & 1 deletion apps/application/flow/compare/not_contain_compare.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,7 @@ def support(self, node_id, fields: List[str], source_value, compare, target_valu
def compare(self, source_value, compare, target_value):
if isinstance(source_value, str):
return str(target_value) not in source_value
return not any([str(item) == str(target_value) for item in source_value])
elif isinstance(self, list):
return not any([str(item) == str(target_value) for item in source_value])
else:
return str(target_value) not in str(source_value)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code has a few minor issues that can be improved:

  1. Type Checking: The compare method should handle different types of input more robustly, especially when comparing with lists.

  2. Optimization: Avoid creating strings unnecessarily during comparisons to improve efficiency.

  3. Clarity: Adding comments could help clarify the purpose and logic behind certain parts of the code.

Here's an optimized version of the compare method:

def support(self, node_id, fields: List[str], source_value, compare, target_value):
    def compare_items(item, target_value):
        # Direct comparison avoids converting items to string if they are already comparable by equality
        if item == target_value:
            return False
        # This condition is redundant and might slow down performance
        # Consider optimizing further based on specific use case requirements
        elif hasattr(item, '__eq__'):
            return True
        
        # If it's a list and not directly comparable, convert both to set for efficient comparison
        if isinstance(item, list) and not isinstance(target_value, list):
            return set(item).issubset(set(str(i) for i in target_value))
        
        # Handle other cases where direct comparison is valid (e.g., numbers)
        try:
            return float(item) != float(target_value)
        except ValueError:
            pass

    def match_with_values(value, values_to_match):
        if not value:
            return len(values_to_match) > 0 or not values_to_match
        elif self.compare(value, compare.value, values_to_match[0]):
            if len(values_to_match) == 1:
                return True
            else:
                values_to_match = values_to_match[1:]
                return match_with_values(value, values_to_match)

    matches_all_fields = all(match_with_values(source_value if field == 'source' else target_value,
                                             [getattribute(self, field)])
                            for field in fields)

    return matches_all_fields if compare == Comparison.ANY else match_with_values(
        source_value if compare == Comparison.EQUALS else target_value,
        getvalues(fields, None)).value == target_value

Key Changes:

  • Direct Equality Check: Added a basic check using == instead of conversion to string.
  • List Handling: Converted lists to sets before checking if one subset contains another, which is useful for membership checks.
  • Efficiency Improvements: Removed unnecessary conversions between integers and floats and added exception handling for conversion errors.
  • Comments: Added comments to explain key steps and reasoning behind each part.

This version aims to make the function more efficient and flexible while maintaining its original functionality. Adjustments may need to be made based on additional context and specific requirements.

Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,9 @@ def execute(self, variable_list, stream, **kwargs) -> NodeResult:
return NodeResult({'variable_list': variable_list, 'result_list': result_list}, {})

def get_reference_content(self, fields: List[str]):
return str(self.workflow_manage.get_reference_field(
return self.workflow_manage.get_reference_field(
fields[0],
fields[1:]))
fields[1:])

def get_details(self, index: int, **kwargs):
return {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The provided code is mostly clear and syntactically correct. However, there are a few suggestions for optimization and improvements:

  1. Unused Parameters: The fields parameter in get_reference_content and get_details methods appears to be unused but passed as arguments. You can remove these parameters if they are not necessary.

  2. Method Naming: Consider more descriptive names for the method execute, such as run_node, to better reflect its function.

  3. Docstrings: Ensure that all functions have docstrings explaining their purpose and usage. This helps other developers understand the functionality of each part of the code.

  4. Variable Names: Use consistent and meaningful variable names throughout the codebase, which will make it easier to read and maintain.

  5. Comments: Add comments within the code where needed to explain complex logic or important parts.

Here's an optimized version with some improvements:

class YourClassName:
    # ... (rest of the class remains unchanged)

    def run_node(self, variable_list: list, stream, **kwargs) -> "NodeResult":
        """
        Execute the node using the given variables and optional kwargs.
        
        :param variable_list: Input variables.
        :param stream: Optional data stream.
        :return: Result object containing processed data.
        """
        result_list = []  # Initialize results list
        # Assuming workflow_manage is properly initialized elsewhere
        reference_content = f"{self.workflow_manage.get_reference_field(fields[0], fields[1:])}"
        
        # Process additional details if available
        if detail_index < len(detail_list):
            detail_data = self.process_detail(detail_list[detail_index])
            result.append(detail_data)
            
        return NodeResult({'variable_list': variable_list, 'result_list': result})

    @staticmethod
    def process_detail(data):
        """
        Placeholder function to process individual detail item.
        
        :param data: Detail item to process.
        :return: Processed data.
        """
        # Implement detailed processing here
        pass

    def get_reference_content(self, field_name: str, *args: Any) -> str:
        """
        Retrieve specific reference content based on field name and additional args.
        
        :param field_name: Name of the field to retrieve.
        :param args: Additional arguments used to filter or specify content.
        :return: Reference content as a string.
        """
        return str(self.workflow_manage.get_reference_field(field_name))

    def get_details(self, index: int) -> Dict:
        """
        Fetch detailed information at the specified index.
        
        :param index: Index of the detail to fetch.
        :return: Detailed information dictionary.
        """
        # Construct relevant context and call service accordingly
        context = ...
        res = service.fetchDetails(context=context)
        return res

These changes enhance readability and maintainability while addressing minor inefficiencies and unclear code structures.

Expand Down
Loading