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 remove node for array operations #609

Merged
merged 3 commits into from
Dec 30, 2024
Merged

add remove node for array operations #609

merged 3 commits into from
Dec 30, 2024

Conversation

mck
Copy link
Collaborator

@mck mck commented Dec 30, 2024

User description

Adds a new array node that removes an item at a specified index and returns both the modified array and the removed item. Supports both positive and negative indices for flexible array manipulation.

  • Added remove.ts node implementation
  • Added comprehensive tests

PR Type

Enhancement, Tests


Description

  • Introduced a new Remove Item node for array operations, allowing removal of an item at a specified index and returning both the modified array and the removed item.
  • Supported both positive and negative indices for flexible array manipulation.
  • Added comprehensive tests to validate the functionality, including edge cases such as out-of-bounds indices and ensuring the original array remains unmodified.
  • Updated the array node exports to include the new Remove Item node.
  • Added a changeset to document the new feature.

Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Add `remove` node to array exports                                             

packages/graph-engine/src/nodes/array/index.ts

  • Added remove to the array node exports.
  • Updated the list of available array nodes.
  • +2/-0     
    remove.ts
    Implement `Remove Item` node for arrays                                   

    packages/graph-engine/src/nodes/array/remove.ts

  • Implemented a new Remove Item node for arrays.
  • Supports removing items by positive and negative indices.
  • Returns the modified array and the removed item.
  • +62/-0   
    Tests
    remove.test.ts
    Add tests for `Remove Item` node                                                 

    packages/graph-engine/tests/suites/nodes/array/remove.test.ts

  • Added tests for the Remove Item node.
  • Verified functionality for positive and negative indices.
  • Ensured the original array remains unmodified.
  • Tested behavior for out-of-bounds indices.
  • +57/-0   
    Documentation
    tidy-humans-melt.md
    Add changeset for `Remove Item` node                                         

    .changeset/tidy-humans-melt.md

  • Documented the addition of the Remove Item node.
  • Highlighted support for flexible array manipulation.
  • +5/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Adds a new array node that removes an item at a specified index and returns both the modified array and the removed item. Supports both positive and negative indices for flexible array manipulation.
    
    - Added remove.ts node implementation
    - Added comprehensive tests
    - Updated array node exports
    @mck mck self-assigned this Dec 30, 2024
    Copy link

    changeset-bot bot commented Dec 30, 2024

    🦋 Changeset detected

    Latest commit: b74a3cf

    The changes in this PR will be included in the next version bump.

    This PR includes changesets to release 1 package
    Name Type
    @tokens-studio/graph-engine Minor

    Not sure what this means? Click here to learn what changesets are.

    Click here if you're a maintainer who wants to add another changeset to this PR

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Edge Case Handling

    Ensure that the implementation correctly handles edge cases such as out-of-bounds indices and empty arrays. Validate that the outputs are consistent and do not cause unexpected behavior.

    execute(): void | Promise<void> {
        const { index, array } = this.getAllInputs();
        const arrayType = this.inputs.array.type;
    
        // Create a copy of the array value
        const result = [...array];
        let removedItem: T | undefined;
    
        if (index >= 0 && index < result.length) {
            // Remove item at positive index
            [removedItem] = result.splice(index, 1);
        } else if (index < 0 && Math.abs(index) <= result.length) {
            // Handle negative indices by counting from the end
            const actualIndex = result.length + index;
            [removedItem] = result.splice(actualIndex, 1);
        }
    
        // Set the outputs
        this.outputs.array.set(result, arrayType);
        if (removedItem !== undefined) {
            this.outputs.item.set(removedItem, arrayType.items);
        }
    }
    Type Safety

    Verify that the type definitions for inputs and outputs are robust and prevent potential runtime errors, especially with the use of AnyArraySchema and AnyArraySchema.items.

    this.addInput('array', {
        type: AnyArraySchema
    });
    this.addInput('index', {
        type: NumberSchema
    });
    this.addOutput('array', {
        type: AnyArraySchema
    });
    this.addOutput('item', {
        type: AnyArraySchema.items

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure the array input is a valid array to avoid runtime errors

    Handle cases where the array input is null or undefined to prevent runtime errors
    during execution.

    packages/graph-engine/src/nodes/array/remove.ts [44]

    +if (!Array.isArray(array)) {
    +    throw new Error('Input must be a valid array');
    +}
     const result = [...array];
    Suggestion importance[1-10]: 10

    Why: This suggestion is highly impactful as it prevents runtime errors by validating that the array input is a valid array before proceeding. The improved code is precise and effectively handles a critical edge case.

    10
    Add validation to ensure the index input is an integer to prevent runtime errors

    Ensure that the index input is validated to be an integer before proceeding with the
    logic to avoid potential runtime errors or unexpected behavior.

    packages/graph-engine/src/nodes/array/remove.ts [40]

     const { index, array } = this.getAllInputs();
    +if (!Number.isInteger(index)) {
    +    throw new Error('Index must be an integer');
    +}
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical issue by ensuring that the index input is validated as an integer, which prevents potential runtime errors or unexpected behavior. The improved code is accurate and directly enhances the robustness of the function.

    9
    Validate arrayType.items before using it to set the output type for item

    Ensure that arrayType.items is validated before being used to set the output type
    for item to avoid potential type errors.

    packages/graph-engine/src/nodes/array/remove.ts [59]

    +if (!arrayType.items) {
    +    throw new Error('Array type does not define items');
    +}
     this.outputs.item.set(removedItem, arrayType.items);
    Suggestion importance[1-10]: 8

    Why: This suggestion improves the reliability of the code by ensuring arrayType.items is validated before use, preventing potential type errors. While not as critical as the previous suggestions, it enhances the robustness of the output handling.

    8

    @mck mck merged commit 24e99b1 into master Dec 30, 2024
    2 checks passed
    @mck mck deleted the remove-item branch December 30, 2024 14:38
    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.

    1 participant