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

handle glob parsing in shell caommands #721

Merged
merged 12 commits into from
Sep 20, 2024
Merged

handle glob parsing in shell caommands #721

merged 12 commits into from
Sep 20, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 20, 2024

This pull request includes updates to the model options, modifications to binary files across multiple paths, and fixes for typos in the prompt_template.d.ts file. The changes in the model options include adding new entries in the settings.json file and fixing typos in the prompt_template.d.ts file. The modifications to binary files are reflected in the git patches provided. Overall, this pull request aims to improve the codebase by updating model options, modifying binary files, and fixing typos.


  • 🆙 Multiple dependencies across various package.json files have been upgraded.
  • 🔄 The function githubParseEnv has been expanded to include async logic to fetch missing repo, owner, and issue data.
  • 💫 A new function githubQueryEnvUsingCli has been introduced but not used.
  • 🔄 Logic for updating Pull Request Reviews and Comments in the runScript function has been updated to asynchronously parse Github environment data.
  • 🔄 The functions githubUpdatePullRequestDescription, githubCreatePullRequestReview, and githubCreateIssueComment have been adjusted to assert the presence of certain variables.
  • 🔄 The azureDevOpsParseEnv function in packages/core/src/azuredevops.ts has been converted to an async function.
  • 🐞 Fixes have been made to the 'Pull Request Description' error reporting.
  • 🎛️ Added new parameter defaultBranch to the pr-describe script in the sample packages. This denotes the default branch of the repository.
  • ♨️ Some code refactoring and typo corrections in packages/core/src/types/prompt_template.d.ts.
  • 🔄 Changes in the User Facing API:
    • The model option in ModelConnectionOptions now accepts an OptionsOrString type allowing the usage of custom strings.
    • Small typo fixes in the documentation comments of this file.

generated by pr-describe


  • 🔧 Dependency Updates: Updated versions for several dependencies across multiple package.json files, such as @astrojs/starlight, gpt-tokenizer, and promptfoo.

  • 🚀 Async Enhancements: Converted several functions to asynchronous, including githubParseEnv and azureDevOpsParseEnv, to likely improve performance and handle asynchronous operations.

  • 🐛 Bug Fixes and Improvements: Added checks and improved logic for GitHub and Azure DevOps interactions, including better handling of repository and issue information.

  • 📜 Documentation and Typo Corrections: Fixed typos and improved documentation comments, ensuring clarity and correctness.

  • 🔄 Command Refactoring: Introduced reusable command variables for Git operations in scripts, enhancing maintainability and readability.

  • 🛠️ Code Quality: Made code style improvements, such as ensuring proper assertions and formatting adjustments, to enhance code quality and consistency.

generated by prd

Copy link

The changes in the pull request primarily focus on the following areas:

  1. shell.ts: The shellParse function is updated to filter out comments and handle glob operations differently. It now returns the pattern instead of "glob".

  2. prompt_template.d.ts:

    • The ModelConnectionOptions interface is updated to change model property from string type to OptionsOrString. This allows wrapping the type in an options object, providing more flexibility.
    • Several comments are updated for accuracy and spelling corrections, like in WorkspaceFile, WorkspaceFileSystem's cache method, ContextExpansionOptions, Parsers' YAML method, and DefDataOptions.
    • The DefDataOptions' comment on output format is updated to clarify that the default is Markdown table rendering.

No functional issues are found in the changes, and the code seems to be improved on clarity and accuracy of comments.

There is no violation of principles of reliability, security, scalability, and performance.

So, my response is "LGTM 🚀".

generated by pr-review

@@ -89,7 +89,7 @@ Options:
-td, --test-delay <string> delay between tests in seconds
--cache enable LLM result cache
-v, --verbose verbose output
-pv, --promptfoo-version [version] promptfoo version, default is 0.82.0
-pv, --promptfoo-version [version] promptfoo version, default is 0.89.3

Choose a reason for hiding this comment

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

The flag --promptfoo-version seems to be a typo; it should be --prompt-version.

generated by pr-docs-review-commit flag_typo

logError(
"pull request review: no pull request information found"
"pull request description: no pull request information found"

Choose a reason for hiding this comment

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

The error message has been changed from "pull request review: no pull request information found" to "pull request description: no pull request information found". Please ensure that this change is intentional and does not affect the debugging process. 🧐

generated by pr-review-commit error_message_change

| "openai:gpt-3.5-turbo"
| "ollama:phi3"
| "ollama:llama3"
| "ollama:mixtral"
| string
>

Choose a reason for hiding this comment

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

The model options have been changed. The new options include "openai:gpt-4o" and "openai:gpt-4o-mini". Please ensure that these models are supported and the change is intentional. 🤔

generated by pr-review-commit model_options_change

@@ -1439,7 +1439,7 @@ interface DefDataOptions
extends Omit<ContextExpansionOptions, "maxTokens">,
DataFilter {
/**
* Output format in the prompt. Defaults to markdownified CSV
* Output format in the prompt. Defaults to Markdown table rendering.

Choose a reason for hiding this comment

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

The comment for the 'format' option in 'DefDataOptions' has been changed from "Output format in the prompt. Defaults to markdownified CSV" to "Output format in the prompt. Defaults to Markdown table rendering.". Please ensure that this change is accurate and does not mislead the users. 📝

generated by pr-review-commit comment_change

} else {
logError(
"pull request description: no pull request information found"
)

Choose a reason for hiding this comment

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

The error handling logic for the case when no pull request information is found has been changed. Previously, it was simply logging an error message. Now, it tries to query GitHub using the CLI and only logs an error if that also fails. This could potentially lead to unhandled exceptions if the GitHub CLI is not installed or not functioning correctly. It would be safer to wrap the new code in a try-catch block and handle any exceptions that might occur. 🛠️

generated by pr-review-commit error_handling

logVerbose("github query failed")
return undefined
}
}

Choose a reason for hiding this comment

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

The function githubQueryEnvUsingCli is using the GitHub CLI to query for repository and issue information. This introduces a new dependency on the GitHub CLI being installed and correctly configured on the system where this code is running. If the GitHub CLI is not available, this function will fail. It would be better to handle this dependency explicitly and provide a clear error message if the GitHub CLI is not available. 📦

generated by pr-review-commit cli_dependency

@@ -425,7 +425,8 @@ export async function runScript(
}

if (pullRequestReviews && result.annotations?.length) {
const info = githubParseEnv(process.env)
// github action or repo

Choose a reason for hiding this comment

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

The function githubParseEnv is now an async function, but it's not being awaited properly. This could lead to unexpected behavior.

generated by pr-review-commit async_function

@@ -436,7 +437,8 @@ export async function runScript(
}

if (pullRequestComment && result.text) {
const info = githubParseEnv(process.env)
// github action or repo
const info = await githubParseEnv(process.env)

Choose a reason for hiding this comment

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

The function githubParseEnv is now an async function, but it's not being awaited properly. This could lead to unexpected behavior.

generated by pr-review-commit async_function

Copy link
Member Author

In the hush of twilight's glow,
Whispers dance where breezes flow.
Stars awaken, skies embrace,
Night unfolds with gentle grace.

generated by poem

@pelikhan pelikhan merged commit af41129 into main Sep 20, 2024
8 of 10 checks passed
@pelikhan pelikhan deleted the modeltest branch September 20, 2024 15:26
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.

1 participant