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

Deps/2024 09 07 14 19 32 469 #697

Closed
wants to merge 2 commits into from
Closed

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 7, 2024

  • There have been some dependency updates in package.json across different folders:

    • In docs/package.json, "@astrojs/starlight" has been updated to "^0.27.0" and "astro" has been updated to "^4.15.4".
    • In packages/cli/package.json and packages/core/package.json, "openai" was updated to "^4.58.1".
  • 🔄 In packages/core/src/globals.ts, a new global function cancel has been introduced which throws an instance of CancelError.

  • 🌟 In packages/core/src/promptcontext.ts, the cancel function has been removed from the prompt context.

  • 🔍 In the public API (packages/core/src/types/prompt_template.d.ts), the declaration of a cancel function has been removed, indicating that cancel isn't any longer part of the expected interface for PromptContext.

  • ☝️ These changes suggest that the method of signaling a cancellation has moved from a cancel function within the PromptContext objects, to using the cancel function available on the global object.

  • 💡 This could be a refactor to centralize the handling and triggering of cancellations in the program, improving flexibility and control over program execution flows.

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 CancelError but it is not being caught anywhere. This could lead to unexpected behavior or crashes. Please ensure that this error is properly handled. 🧐

generated by pr-review-commit unhandled_exception

@@ -237,9 +221,6 @@ export async function createPromptContext(
defFileMerge: (fn) => {
appendPromptChild(createFileMerge(fn))
},
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 is being used elsewhere in the code, it could lead to undefined behavior or crashes. Please ensure that all references to this function have been updated or removed. 😮

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 function has been removed from the PromptContext interface. This could break any implementations of this interface that rely on this function. Please ensure that all implementations of this interface have been updated. 😱

generated by pr-review-commit removed_functionality

Copy link

github-actions bot commented Sep 7, 2024

The changes involve some refactoring and rearrangement of functions in the TypeScript files. Here's a summary of the changes:

  • In globals.ts, a new function cancel() is added which throws a CancelError with a default or a provided string as reason.
  • In promptcontext.ts, some import statements are removed which indicates that these dependencies are no longer needed in this file. The function cancel() here is also removed, presumably because it's now globally available.
  • In prompt_template.d.ts, the cancel() function is removed from the PromptContext interface, again presumably because it's now globally accessible.

Without more context, it's hard to provide a functional review. However, a possible concern could be if any code which is still relying on the cancel() function to be present directly in the PromptContext interface instead of being globally available.

If the code has been properly refactored to accommodate these changes, then it looks good to me. LGTM 🚀

generated by pr-review

@pelikhan pelikhan closed this Sep 7, 2024
@pelikhan pelikhan deleted the deps/2024-09-07-14-19-32-469- branch October 4, 2024 01:16
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