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

Ravi praveen patch 2 #618

Closed
wants to merge 4 commits into from

Conversation

RAVI-PRAVEEN
Copy link

@RAVI-PRAVEEN RAVI-PRAVEEN commented Oct 8, 2024

Summary of Changes:

Granular System Definition: Introduced CreateUserSystemDef as an example of a more specific system call.
Custom Tool Type: Added a custom option to the ToolType enum for future extensibility.
API Call Enhancements: Added fields for retries, rate_limit, page_size, and page_number to ApiCallDef for better error handling and pagination.
Function Parameters: Refined FunctionParameters with more structure and validation (type, required, default).
Tool Logging: Added a log field in the Tool model for logging tool actions (e.g., user, action time).
Improved Comments and Documentation: Added comments to fields for better clarity.


Important

Enhances system and tool definitions with new models, error handling, and logging, and updates script for better logging and error handling.

  • System and Tool Enhancements:
    • Added CreateUserSystemDef model for specific user creation system calls in models.tsp.
    • Introduced custom option in ToolType enum for future extensibility in models.tsp.
    • Enhanced ApiCallDef with fields for retries, rate_limit, page_size, and page_number for better error handling and pagination in models.tsp.
    • Refined FunctionParameters with structured validation in models.tsp.
    • Added log field in Tool model for logging actions in models.tsp.
  • Script Enhancements:
    • Updated generate_openapi_code.sh to include logging with timestamps and error handling.
    • Added options for updating Poetry dependencies and displaying usage information in generate_openapi_code.sh.

This description was created by Ellipsis for 9034dd7. It will automatically update as commits are pushed.

Key Changes:
Modular Functions: The script is now organized into functions for logging actions, compiling TypeSpec, and running Poetry tasks.
Logging: Introduced log_action for logging actions and errors with timestamps. This helps track what the script is doing in real-time.
Error Handling: Added || { ... } constructs to gracefully handle errors and log them.
Optional Poetry Update: The --update-poetry flag is available to update Poetry dependencies before running tasks. If not passed, Poetry runs without updating.
Execution Time: Logs the total time taken for the script to execute, making it easier to track performance.
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 9034dd7 in 19 seconds

More details
  • Looked at 207 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. scripts/generate_openapi_code.sh:7
  • Draft comment:
    Redundant 'set -e' as 'set -xe' already includes 'set -e'. Consider removing this line.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The script uses 'set -xe' and 'set -e' which is redundant. 'set -e' is already included in 'set -xe'.
2. scripts/generate_openapi_code.sh:14
  • Draft comment:
    Consider removing 'tee' if console output is not needed, as it duplicates the log entry to both the console and the log file.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The 'log_action' function is used to log actions with timestamps, but the 'tee' command is used, which might not be necessary if the output is only needed in the log file.
3. typespec/tools/models.tsp:49
  • Draft comment:
    Consider using a more specific type instead of 'Record' for 'ToolOutput' to improve type safety and clarity. This comment also applies to 'data' in 'ApiCallDef'.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The 'FunctionParameters' alias in 'models.tsp' is defined with a specific structure, but the 'Record' type for 'ToolOutput' and 'data' in 'ApiCallDef' is too generic and might benefit from more specific typing.
4. typespec/tools/models.tsp:125
  • Draft comment:
    Consider including 'arguments?: FunctionParameters;' in 'CreateUserSystemDef' for consistency with 'SystemDef'.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The 'CreateUserSystemDef' model extends 'SystemDef' but does not include 'arguments', which might be necessary for consistency and functionality.

Workflow ID: wflow_0KSUWYEYwL5ZVcIL


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@creatorrr
Copy link
Contributor

creatorrr commented Oct 8, 2024

Hi @RAVI-PRAVEEN ,

- We are not accepting changes to typespec models unless absolutely necessary.
- Have you verified if the bash script works correctly? Please post output here that it does and I'd be happy to merge this pr

Closing as duplicate of #615

@creatorrr creatorrr mentioned this pull request Oct 8, 2024
@creatorrr creatorrr closed this Oct 8, 2024
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