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

#313 Update generate_openapi_code.sh #547

Closed
wants to merge 6 commits into from

Conversation

Piyushsahu99
Copy link

@Piyushsahu99 Piyushsahu99 commented Oct 2, 2024

The script has been enhanced by organizing the tasks into functions for improved readability and modularity. A logging system was introduced to capture and store actions in both the console and a log file for better traceability. Error handling was added to ensure that any failures are caught with custom error messages, improving the robustness of the script. Additionally, an optional feature allows updating Poetry dependencies via a command-line flag, adding flexibility. The script now tracks the total execution time, providing insights into performance. Overall, the improvements make the script more structured, flexible, and user-friendly.


Important

Refactor generate_openapi_code.sh to improve modularity, add logging, error handling, and optional Poetry updates.

  • Modularity:
    • Refactor script into functions: log_action(), compile_typespec(), run_poetry_tasks(), and usage().
  • Logging:
    • Introduce log_action() to log actions to console and script.log.
    • Log start and end of script execution with timestamps.
  • Error Handling:
    • Add error handling with custom messages in compile_typespec() and run_poetry_tasks().
  • Features:
    • Add command-line flag --update-poetry to optionally update Poetry dependencies.
    • Track and log total execution time.
  • Misc:
    • Update script usage instructions in usage().

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

The script has been enhanced by organizing the tasks into functions for improved readability and modularity. A logging system was introduced to capture and store actions in both the console and a log file for better traceability. Error handling was added to ensure that any failures are caught with custom error messages, improving the robustness of the script. Additionally, an optional feature allows updating Poetry dependencies via a command-line flag, adding flexibility. The script now tracks the total execution time, providing insights into performance. Overall, the improvements make the script more structured, flexible, and user-friendly.
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 9f5d9c8 in 10 seconds

More details
  • Looked at 104 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 4 drafted comments based on config settings.
1. scripts/generate_openapi_code.sh:4
  • Draft comment:
    Consider making the -x option conditional based on a debug flag to reduce verbosity in production use.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The script uses set -xe which combines both -x and -e options. The -x option is for debugging and prints each command before executing it, while -e causes the script to exit immediately if a command exits with a non-zero status. This is a good practice for debugging and ensuring the script stops on errors, but it might be too verbose for production use. Consider making the -x option conditional based on a debug flag.
2. scripts/generate_openapi_code.sh:18
  • Draft comment:
    Use log_action for error messages to ensure they are logged to the file as well.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The error messages in the script are directly echoed to the console. It would be more consistent to use the log_action function for logging errors as well, which would ensure all messages are logged to the file as well as the console.
3. scripts/generate_openapi_code.sh:20
  • Draft comment:
    Ensure cd - > /dev/null is used consistently to suppress output when returning to the previous directory.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The script uses cd - > /dev/null to return to the previous directory and suppress output. This is a good practice to keep the console output clean, but it should be consistent across the script.
4. scripts/generate_openapi_code.sh:19
  • Draft comment:
    Use log_action for all error messages for consistency. This applies to all similar cases in the script.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The script uses echo for error messages, which should be consistent with the logging mechanism used for other messages. This applies to all error messages in the script.

Workflow ID: wflow_yPaRLbnsTLQPw7jN


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

@creatorrr
Copy link
Contributor

@Piyushsahu99 have you tested the script? Try running bash scripts/generate_openapi_code.sh in the root of the repo, if everything is correct, it should not make any changes

@Piyushsahu99
Copy link
Author

Can you pls explain what more changes need to implement to get my pr acceptd.

@creatorrr
Copy link
Contributor

@Piyushsahu99 have you tested the script? Try running bash scripts/generate_openapi_code.sh in the root of the repo, if everything is correct, it should not make any changes

@Piyushsahu99 have you tested the script? Try running bash scripts/generate_openapi_code.sh in the root of the repo, if it run without errors, happy to accept repo. Until then, turning this into a draft pr

@creatorrr creatorrr marked this pull request as draft October 10, 2024 23:41
@creatorrr
Copy link
Contributor

Closing due to inactivity

@creatorrr creatorrr closed this Oct 20, 2024
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.

2 participants