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 runPrompt usage and remove fetchText and cancel from import prompts #700

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 9, 2024

This pull request refactors the usage of the runPrompt function in the codebase and removes the fetchText and cancel parameters from the import prompts. This improves the code by simplifying the prompt generation process and removing unnecessary parameters.

  • Updates were made to importPrompt function in packages/core/src/importprompt.ts, specifically the removal of fetchText and cancel elements.
  • Significant updates were made in packages/core/src/types/prompt_template.d.ts. The function runPrompt() got moved from the PromptContext interface to the ChatGenerationContext interface. This change seems to be a restructuring of the hierarchy of these interfaces.
    • 🔄 In sum, runPrompt() got a new home! 🏡
  • In packages/sample/genaisrc/llm-as-expert.genai.mts, runPrompt() function calls got updated to use a new instance env.generator.runPrompt. This implies an architectural change in the app, defining more specific origins for the runPrompt function calls.
    • 🚀 This could potentially lead to improved code clarity and maintainability in the long run!
  • A similar pattern is seen in packages/sample/genaisrc/pr-create.genai.mts, though the finishReason was also removed from the destructured return of runPrompt().
    • 🔧 It seems like finishReason may no longer be part of the return contract of runPrompt(), or simply isn't needed here.
  • Overall, these changes seem to point towards a clear effort to refactor and tidy up the project's codebase, likely making it more understandable and manageable for future development efforts.
    • 🌟 Applause for good housekeeping!

generated by pr-describe

@@ -23,8 +23,6 @@ export async function importPrompt(
"parsers",
"env",
"retrieval",
"fetchText",
"cancel",
"runPrompt",
Copy link

Choose a reason for hiding this comment

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

The functions fetchText and cancel have been removed. If these functions are being used elsewhere in the code, this could lead to undefined function errors. Please ensure that these functions are either not being used elsewhere, or that appropriate replacements have been provided. 🕵️‍♀️

generated by pr-review-commit removed_functions

@@ -2263,10 +2267,6 @@ interface PromptContext extends ChatGenerationContext {
system(options: PromptSystemArgs): void
defFileMerge(fn: FileMergeHandler): void
defOutputProcessor(fn: PromptOutputProcessorHandler): void
Copy link

Choose a reason for hiding this comment

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

The function runPrompt has been removed from the PromptContext interface. If this function is being used in any implementations of this interface, this could lead to undefined function errors. Please ensure that this function is either not being used in any implementations, or that an appropriate replacement has been provided. 🧐

generated by pr-review-commit removed_function

runPrompt(
generator: string | PromptGenerator,
options?: PromptGeneratorOptions
): Promise<RunPromptResult>
Copy link

Choose a reason for hiding this comment

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

The function runPrompt has been added to the ChatGenerationContext interface. If this function is not implemented in all classes that implement this interface, this could lead to unimplemented function errors. Please ensure that this function is implemented in all classes that use this interface. 🚀

generated by pr-review-commit added_function

Copy link

github-actions bot commented Sep 9, 2024

The pull request mainly involves moving the runPrompt method from the PromptContext interface to the ChatGenerationContext interface within the prompt_template.d.ts file. Additionally, "fetchText" and "cancel" have been removed from the importPrompt function in importprompt.ts.

These changes are likely made for architecture reasons or to improve the structure of the program, potentially optimizing how runPrompt is used throughout the codebase. However, it's important to ensure all instances where runPrompt is called have been updated to reflect this change, otherwise it could cause runtime errors.

Without context on the reason for these changes and the overall codebase, I cannot definitively say if these changes are good or not. However, the changes seem to be relatively simple and straightforward if the above mentioned points are handled appropriately.

Therefore, my response is "LGTM 🚀", with the caveat that the developer ensures all references to runPrompt are updated and the removal of 'fetchText' and 'cancel' does not affect any dependencies.

generated by pr-review

@pelikhan pelikhan merged commit bbab058 into main Sep 9, 2024
11 checks passed
@pelikhan pelikhan deleted the run-prompt-generator branch September 9, 2024 14:28
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