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

Move cancel function implementation from promptcontext to globals #696

Merged
merged 1 commit into from
Sep 7, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 7, 2024

  • The cancel method has been moved from the PromptContext object to the global scope of the application. This is evident from the changes in 'packages/core/src/globals.ts' and 'packages/core/src/promptcontext.ts' files. This could be to make cancel functionality universally accessible. 🌎
  • Consequently, the cancel method was removed from the PromptContext interface definition in the 'packages/core/src/types/prompt_template.d.ts' file. This is a user facing change as it modifies how users interact with the 'PromptContext' interface. ⚠️
  • The createPromptContext function no longer throws a CancelError exception; this responsibility seems to have been shifted to the moved cancel method. Therefore, the code might be becoming more modular or adhering to separation of concerns principle. 📏
  • Note, no other functions or methods were introduced or significantly altered, indicating the primary purpose of these changes was around organizing handling of cancel operations. ♻️

generated by pr-describe

@@ -62,4 +63,7 @@ export function installGlobals() {
convertToMarkdown: HTMLToMarkdown,
convertToText: HTMLToText,
})
glb.cancel = (reason?: string) => {
throw new CancelError(reason || "user cancelled")
}
Copy link

Choose a reason for hiding this comment

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

You are throwing a new CancelError in the cancel function but it's not being caught anywhere. This could lead to an unhandled promise rejection. Please make sure to catch this error where the cancel function is being called. 🚀

generated by pr-review-commit unhandled_rejection

@@ -237,9 +221,6 @@ export async function createPromptContext(
defFileMerge: (fn) => {
appendPromptChild(createFileMerge(fn))
},
cancel: (reason?: string) => {
throw new CancelError(reason || "user cancelled")
},
runPrompt: async (generator, runOptions): 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 cancel function has been removed from the PromptContext. If this function was being used elsewhere in the code, it could lead to a TypeError of cancel is not a function. Please ensure that the cancel function is not being used elsewhere in the code or provide an alternative implementation. 🧐

generated by pr-review-commit removed_functionality

@@ -2276,7 +2276,6 @@ interface PromptContext extends ChatGenerationContext {
text?: string
Copy link

Choose a reason for hiding this comment

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

The cancel method has been removed from the PromptContext interface. This change could break existing implementations that rely on this method. Please ensure that this change is intentional and that all implementations of this interface have been updated accordingly. 🕵️‍♀️

generated by pr-review-commit interface_change

Copy link

github-actions bot commented Sep 7, 2024

The changes in the GIT_DIFF involve two main adjustments in the TypeScript code:

  1. In globals.ts, the installGlobals method has been updated to include a cancel function that throws a CancelError. This is a new global function available to the rest of the code.

  2. In promptcontext.ts, several import statements have been removed, and the previously defined cancel function in the createPromptContext method has also been removed. This suggests that the global cancel function in globals.ts will replace the local cancel function previously defined here.

  3. In prompt_template.d.ts, the cancel method has been removed from the PromptContext interface.

There don't appear to be any major issues with these changes. It looks like a refactoring effort to globalize the cancel function and reduce code duplication. However, there are some points to consider:

  • The CancelError being thrown in globals.ts has a default message user cancelled. This might take away some granularity in error reporting if different parts of the code relied on specific error messages when cancellation occurs.
  • There might be a potential side effect if any code relies on the absence of a cancel method in PromptContext interface as a condition.

Otherwise, LGTM 🚀

generated by pr-review

@pelikhan pelikhan merged commit ba1d047 into main Sep 7, 2024
11 checks passed
@pelikhan pelikhan deleted the moreglobal branch September 7, 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