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

refactor: add required=True to necessary components #5679

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

raphaelchristi
Copy link
Contributor

This pull request adds required=True to essential component fields to improve input validation.

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. enhancement New feature or request labels Jan 14, 2025
@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed enhancement New feature or request labels Jan 14, 2025
@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 14, 2025
@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 14, 2025
Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #5679 will degrade performances by 24.72%

Comparing raphaelchristi:feature/update-required-params (94c8330) with main (125a1c1)

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 13 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main raphaelchristi:feature/update-required-params Change
test_successful_run_with_input_type_any 258.8 ms 182.2 ms +42.02%
test_successful_run_with_input_type_text 253.3 ms 336.4 ms -24.72%

@edwinjosechittilappilly
Copy link
Collaborator

@erichare Any suggestions on the Astra Component?

@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 17, 2025
@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 17, 2025
Copy link
Collaborator

@edwinjosechittilappilly edwinjosechittilappilly left a comment

Choose a reason for hiding this comment

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

Remove required is true for URL input (since when used in tool mode it gives error).

General suggestion if the component is being used as a tool i suggest that field need not be set required as true.

@ogabrielluiz what is your suggestion? or should we handle this in component toolkit?

Each component need to be tested in tool mode, when required fields are set

@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 19, 2025
@github-actions github-actions bot removed the refactor Maintenance tasks and housekeeping label Jan 19, 2025
@github-actions github-actions bot added the refactor Maintenance tasks and housekeeping label Jan 19, 2025
@github-actions github-actions bot added refactor Maintenance tasks and housekeeping and removed refactor Maintenance tasks and housekeeping labels Jan 19, 2025
@ogabrielluiz
Copy link
Contributor

Remove required is true for URL input (since when used in tool mode it gives error).

General suggestion if the component is being used as a tool i suggest that field need not be set required as true.

@ogabrielluiz what is your suggestion? or should we handle this in component toolkit?

Each component need to be tested in tool mode, when required fields are set

I agree that we should test it but I don't think it should error if the input is a tool_mode=True field. If the field is used in tool mode, and it is not available for the user to add data to it, then it should work normally.

Copy link
Collaborator

@edwinjosechittilappilly edwinjosechittilappilly left a comment

Choose a reason for hiding this comment

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

This is a solid PR overall, but it introduces significant breaking changes. Before merging, we should address a few key points:
• We need to add more comprehensive tests, particularly for scenarios where tool_mode is set to true and required cannot be true at the same time.
@carlosrcoelho: Can we explore a more refined solution here? Additionally, if feasible, let’s define tasks for specific utilities that can help us thoroughly test these breaking cases.

Let’s revisit this PR after resolving these concerns to ensure a smooth integration.

@anovazzi1 anovazzi1 removed their request for review January 20, 2025 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Maintenance tasks and housekeeping 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