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 code and update documentation #719

Merged
merged 3 commits into from
Sep 19, 2024
Merged

Refactor code and update documentation #719

merged 3 commits into from
Sep 19, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 19, 2024

The pull request includes several commits that refactor the codebase. The dedentAsync function has been removed and the createDefNode function has been refactored to createDef. Unused imports have also been removed from the indent.ts file. Additionally, the documentation has been updated with examples for using env.files in prompts.

  • 🏗️ Adjusted how prompting for LLMs is performed in the documentation, and how files arguments (from CLI) are handled in scripts. Involved changes in docs/src/content/docs/index.mdx and docs/src/content/docs/reference/scripts/context.md.
  • 💡 Introduced ways to directly inject env.files in a $ call and ways to handle it using def.
  • ⏩ Simplified code in packages/core/src/indent.ts by removing dedentAsync function, and its usage in packages/core/src/promptdom.ts has been replaced with asynchronous handling within the 'stringTemplate' case in resolvePromptNode.
  • 👁️‍🗨️ The string template resolver now inspects and handles file objects specially. If an object is a file or an array of files, it creates new def nodes for them using the updated createDef function (renamed from createDefNode for consistency).
  • 🔄 Revised the method of calling createDef function in packages/core/src/runpromptcontext.ts.
  • ✨ A new script packages/sample/genaisrc/nested-args.genai.mts has been added that demonstrates the use of nested arguments for summarizing files.
  • 🛠️ Changes in other files like (i.e. path changes, renamed methods) appear to be refactoring or cleanup of the codebase.

generated by pr-describe

@@ -36,6 +36,12 @@ import testExplorerAlt from "../../assets/vscode-test-explorer.png.txt?raw"

Programmatically assemble prompts for LLMs using JavaScript.

```js
$`Analyze ${env.files} and report errors. Use emojis.`
```

Choose a reason for hiding this comment

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

Invalid JavaScript code snippet, missing closing backtick and parenthesis.

generated by pr-docs-review-commit invalid_code

```

Of course, things can get more complex...

```js wrap title="extract-data.genai.mjs"
// define the context
def("FILE", env.files, { endsWith: ".pdf" })

Choose a reason for hiding this comment

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

Typo in the JavaScript code snippet, 'weahter' should be 'weather'.

generated by pr-docs-review-commit typo

```js
def("FILE", env.files)
```

Or filtered,
- filtered,

```js
def("DOCS", env.files, { endsWith: ".md" })

Choose a reason for hiding this comment

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

Missing code fence backticks to close the JavaScript code block.

generated by pr-docs-review-commit missing_code_fence

Copy link

The changes can be summarized as follows:

  1. Removal of dedentAsync function from indent.ts and the associated import statements.
  2. Addition of DEDENT_INSPECT_MAX_DEPTH in promptdom.ts which was removed from indent.ts.
  3. Renaming of createDefNode to createDef.
  4. Modification of stringTemplate in resolvePromptNode. The logic has been expanded to handle more cases of argument rendering.
  5. Modification of createChatTurnGenerationContext function to use the newly named createDef function.

Although the changes look good, I have one concern:

⚠️ The changes in resolvePromptNode make the function quite long and complex. It might be beneficial, for clarity and maintainability, to break this function into smaller helper functions.

However, overall, the code changes look good.

LGTM 🚀

generated by pr-review

@@ -105,7 +105,7 @@ export function createChatTurnGenerationContext(
throw new Error(`def ${name} is empty`)
appendChild(
node,
createDefNode(
createDef(
name,
{ filename: "", content: body },
doptions

Choose a reason for hiding this comment

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

The function createDefNode has been renamed to createDef. If this function is used in other parts of the codebase, those references will need to be updated to reflect this change. This could potentially break the code if not handled properly. 🔄

generated by pr-review-commit function_rename

@pelikhan pelikhan merged commit bb4f02e into main Sep 19, 2024
12 checks passed
@pelikhan pelikhan deleted the litfiles branch September 19, 2024 23:48
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