-
Notifications
You must be signed in to change notification settings - Fork 126
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
Notebook support #540
Notebook support #540
Conversation
/genai-describe /genai-review |
docs/src/content/docs/getting-started/your-first-genai-script.mdx
Outdated
Show resolved
Hide resolved
LGTM 🚀 The changes in the pull request include:
All changes seem to be well implemented and should not introduce any functional issues. 👍
|
packages/vscode/src/notebook.ts
Outdated
} | ||
|
||
return combined | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function getBetweenCellsWhitespace
is defined but never used. Consider removing it if it's not needed.
generated by pr-review-commit
unused_function
|
||
```md title="output" wrap | ||
Beneath the silver moon's gentle gaze, the tranquil sea whispers secrets to the shore. | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wrap
attribute in the markdown code block is unnecessary and not standard markdown syntax.
generated by pr-docs-review-commit
unnecessary_wrap_attribute
packages/vscode/src/notebook.ts
Outdated
indentation | ||
) | ||
} | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is deeply nested which makes it hard to read and understand. Consider refactoring to reduce the level of nesting. 🔄
generated by pr-review-commit
deep_nesting
packages/vscode/src/notebook.ts
Outdated
} | ||
|
||
return cells | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function parseMarkdown
is quite long and does a lot of things. Consider breaking it down into smaller, more manageable functions. This will improve readability and maintainability. 📚
generated by pr-review-commit
long_function
The execution of the GenAIScript generates a prompt (and more) | ||
that gets sent to the LLM model. | ||
|
||
The ` $``...`` ` template string function formats and write the string to the prompt; which gets sent to the LLM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template string function is incorrectly represented as $``...```. It should be
${...}` to correctly denote a template literal in JavaScript.
generated by pr-docs-review-commit
template_literal
|
||
```js title="poem.genai.js" | ||
$`Write a one sentence poem.` | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code block starting on line 50 should specify the language for syntax highlighting. It should be js instead of
js title="poem.genai.js".
generated by pr-docs-review-commit
code_fence_info_string
|
||
```md title="🤖" wrap | ||
In the quiet dusk, the willow whispers secrets to the passing breeze. | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code block starting on line 54 should specify the language for syntax highlighting. It should be md instead of
md title="🤖" wrap. Additionally, the "wrap" option is not standard and may not be recognized by the renderer.
generated by pr-docs-review-commit
code_fence_info_string
|
||
get project() { | ||
return this.file.project | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project
getter method has been removed from the Fragment
class but there is no replacement provided. If other parts of the codebase are using this method, this will cause a breakage.
generated by pr-review-commit
unused_code
@@ -28,6 +27,7 @@ import { fenceMD } from "./markdown" | |||
import { YAMLStringify } from "./yaml" | |||
import { estimateChatTokens } from "./tokens" | |||
import { createChatTurnGenerationContext } from "./runpromptcontext" | |||
import { dedent } from "./indent" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import of dedent
has been changed from ts-dedent
to ./indent
. If the implementation of dedent
in ./indent
is different from ts-dedent
, this could potentially introduce bugs.
generated by pr-review-commit
import_change
@@ -350,7 +367,7 @@ temp/ | |||
} else if (connectionToken.type === "localai") await startLocalAI() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The conditional logic for setting cliInfo
in the genOptions
object is quite complex and hard to understand. Consider simplifying this logic or breaking it down into smaller functions for better readability and maintainability.
generated by pr-review-commit
conditional_complexity
|
||
```md title="🤖" wrap | ||
In the quiet dusk, the willow whispers secrets to the passing breeze. | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code fence for the markdown block is incorrectly formatted; it should not include the title and wrap annotations.
generated by pr-docs-review-commit
incorrect_code_fence
startPos: [0, 0], | ||
endPos: stringToPos(""), | ||
} | ||
await state.requestAI({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing import for PromptScript.
generated by pr-review-commit
missing_import
endPos: stringToPos(""), | ||
} | ||
await state.requestAI({ | ||
template, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing import for PromptParameters.
generated by pr-review-commit
missing_import
.devcontainer/devcontainer.json
.demo/genaisrc/poem.genaibook
has been created. It appears to contain a script for generating a one-sentence poem.demo/web-app-basic-linux/summary_and_diagrams.md
file, including the addition of a new JavaScript script for generating a one-sentence poem.docs/src/content/docs/getting-started/your-first-genai-script.mdx
has been created. It appears to contain documentation for creating a GenAI script.packages/core/src
directory. These changes seem to be related to the handling of file types and indentation.packages/sample/src/jupytest.ipynb
has been created. It appears to be a Jupyter notebook file.packages/sample/src/node.nnb
has been created. It appears to be a notebook file for Node.js.packages/sample/src/poem.genaibook
has been created. It appears to contain a script for generating a one-sentence poem.packages/sample/genaisrc/pr-review-commit.genai.js
file. These changes seem to be related to the guidelines for reviewing pull request commits.