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

Add support for Fenced type in def function and fix snippet content extraction in blog generator #681

Merged
merged 2 commits into from
Sep 1, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 31, 2024

This pull request adds support for the Fenced type in the def function and fixes the snippet content extraction in the blog generator. The changes include modifying the def function to accept the Fenced type as a parameter and updating the blog generator to correctly extract the content from fenced snippets. These changes improve the functionality and accuracy of the code.

  • In the runpromptcontext.ts file, a new conditional block has been added that caters to body objects of type Fenced 🚧. If body is an object and has a content property, a new DefNode is created and appended to the node. The language and other options are passed while creating the DefNode 🎨.

  • For the def method in both prompt_template.d.ts and prompt_type.d.ts (your user-facing API files 👥), the body argument's type lineup has a new entrant - Fenced 🎟. While the function could earlier receive a string or a file or an array of files from the workspace, or a ShellOutput, it can now also accept an object of type Fenced 🔧.

Get ready to interact with Fenced objects in a whole new way! 🚀

generated by pr-describe

Copy link

The pull request primarily introduces the ability to handle Fenced type as a body in the def function, which previously only accepted types string, WorkspaceFile, WorkspaceFile[], or ShellOutput. This change is reflected in both runpromptcontext.ts and in the type definitions in prompt_template.d.ts and prompt_type.d.ts.

These changes look good overall, I don't see any issues with the introduced logic. However, it's important to ensure the Fenced type is being correctly handled elsewhere in the codebase where the def function is being called.

LGTM 🚀

generated by pr-review

{ filename: "", content: (body as Fenced).content },
{ language: fenced.language, ...(doptions || {}) }
)
)

Choose a reason for hiding this comment

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

There is no error handling for the case when body is an object but does not have a content property. This could lead to unexpected behavior or crashes. Consider adding error handling or a default case. 😊

generated by pr-review-commit missing_error_handling

@@ -1451,7 +1451,7 @@ interface ChatTurnGenerationContext {
fence(body: StringLike, options?: FenceOptions): void
def(
name: string,
body: string | WorkspaceFile | WorkspaceFile[] | ShellOutput,
body: string | WorkspaceFile | WorkspaceFile[] | ShellOutput | Fenced,

Choose a reason for hiding this comment

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

The type Fenced is used but not defined in this file or imported from another file. This could lead to confusion or errors. Please ensure that all types are properly defined or imported. 😊

generated by pr-review-commit missing_type_definition

@@ -51,7 +51,7 @@ declare function fence(body: StringLike, options?: FenceOptions): void
*/
declare function def(
name: string,
body: string | WorkspaceFile | WorkspaceFile[] | ShellOutput,
body: string | WorkspaceFile | WorkspaceFile[] | ShellOutput | Fenced,

Choose a reason for hiding this comment

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

The type Fenced is used but not defined in this file or imported from another file. This could lead to confusion or errors. Please ensure that all types are properly defined or imported. 😊

generated by pr-review-commit missing_type_definition

@pelikhan pelikhan merged commit ef30835 into main Sep 1, 2024
8 of 9 checks passed
@pelikhan pelikhan deleted the deffenced branch September 1, 2024 15:11
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