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

Commentor #723

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Commentor #723

merged 11 commits into from
Sep 24, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 24, 2024


  • The JSONLineCache class in cache.ts was expanded with additional comments for better code understanding. 📝
  • Additions in cancellation.ts include remarks for constructors and methods to enhance clarity and maintainability of the code. ✅
  • More in-depth commenting was also appended to classes and methods in chatcache.ts, making it simpler for other engineers to grasp the code's purpose and functionality. 👥
  • Amendments in changelog.test.ts and diff.test.ts suggest an improvement in test coverage. 🧪
  • The 'changelog-genai.js` script is slightly adjusted to correctly detect and handle Changelog blocks. 🔄
  • In glob.ts, the function isGlobMatch now accepts an array of patterns for greater flexibility. ➕
  • runpromptcontext.ts was improved to accept an array of patterns for fileOutput. 🎛
  • In systems.ts, tools resolution was improved to return system IDs linked to specific tools. 🔧
  • TOMLTryParse function in toml.ts now has better error handling to prevent crashes on parsing errors. 💪
  • For prompt type definition files prompt_template.d.ts and prompt_type.d.ts, the defFileOutput function has been modified to accept either a string or an array of strings for pattern specification. 👌
  • A new file cmt.genai.mts has been added, this is a script using generative AI to add comments to other source code files. 🤖

generated by pr-describe

@@ -160,7 +160,7 @@ index N in the above snippets, and then be prefixed with exactly the same whites
the original snippets above. See also the following examples of the expected response format.

CHANGELOG:
\`\`\`
\`\`\`changelog

Choose a reason for hiding this comment

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

Incorrect code fence; 'changelog' language specifier should be removed.

generated by pr-docs-review-commit incorrect_code_fence

@@ -217,9 +217,12 @@ export function applyLLMPatch(source: string, chunks: Chunk[]): string {
const line =
chunk.state === "deleted" ? undefined : chunk.lines[li]
const linei = chunk.lineNumbers[li] - 1
if (isNaN(linei)) throw new DiffError("missing line number")
if (isNaN(linei))

Choose a reason for hiding this comment

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

The error message thrown when the line number is NaN is not descriptive enough. Consider providing more context in the error message.

generated by pr-review-commit missing_error_message

@@ -217,9 +217,12 @@ export function applyLLMPatch(source: string, chunks: Chunk[]): string {
const line =
chunk.state === "deleted" ? undefined : chunk.lines[li]
const linei = chunk.lineNumbers[li] - 1
if (isNaN(linei)) throw new DiffError("missing line number")
if (isNaN(linei))
throw new DiffError(`diff: missing or nan line number`)
if (linei < 0 || linei >= lines.length)

Choose a reason for hiding this comment

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

The error message thrown when the line number is invalid is not descriptive enough. Consider providing more context in the error message.

generated by pr-review-commit missing_error_message

return unique(systems.filter((s) => !!s))
}

// Helper function to resolve tools in the project and return their system IDs
// Finds systems in the project associated with a specific tool
function resolveTools(prj: Project, tool: string): string[] {

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 where the tool is not found in the project. Consider adding error handling to provide a more robust solution.

generated by pr-review-commit missing_error_handling

Copy link

The changes in the pull request primarily involve commenting the code for better understanding. It also includes minor code improvements such as:

  1. The isGlobMatch function in glob.ts has been updated to accept an array of patterns. This change makes the function more flexible and reduces the need for multiple function calls when there are multiple patterns.

  2. A check has been added in promptrunner.ts to ensure that 'changelog' systems are triggered based on both the fenced name and language. This is likely to improve the accuracy of system selection.

  3. The resolveSystems function in systems.ts has been updated to handle edge cases more effectively.

  4. Parsing improvements in toml.ts to improve error handling.

  5. Adjustments in prompt_type.d.ts and prompt_template.d.ts to accept an array for file patterns, which adds flexibility to the input types for the defFileOutput function.

All these changes seem to improve the flexibility, readability and reliability of the code without introducing any apparent functional issues.

So, my response is: LGTM 🚀

generated by pr-review

@@ -28,20 +46,33 @@ export function renderMessageContent(
| ChatCompletionToolMessageParam
): string {
const content = msg.content

Choose a reason for hiding this comment

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

Missing semicolon

generated by pr-review-commit missing_semi

throw new DiffError("invalid line number")
throw new DiffError(
`diff: invalid line number ${linei} in ${lines.length}`
)
lines[linei] = line
}
})

Choose a reason for hiding this comment

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

Missing semicolon

generated by pr-review-commit missing_semi

})
return match
}

Choose a reason for hiding this comment

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

Missing semicolon

generated by pr-review-commit missing_semi

@@ -217,9 +217,12 @@ export function applyLLMPatch(source: string, chunks: Chunk[]): string {
const line =
chunk.state === "deleted" ? undefined : chunk.lines[li]
const linei = chunk.lineNumbers[li] - 1
if (isNaN(linei)) throw new DiffError("missing line number")
if (isNaN(linei))
throw new DiffError(`diff: missing or nan line number`)
if (linei < 0 || linei >= lines.length)

Choose a reason for hiding this comment

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

Missing error handling for potential failure of the minimatch function.

generated by pr-review-commit missing_error_handling

export function isGlobMatch(filename: string, patterns: string | string[]) {
return arrayify(patterns).some((pattern) => {
const match = minimatch(filename, pattern, {
windowsPathsNoEscape: true,

Choose a reason for hiding this comment

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

Missing error handling for potential failure of the minimatch function.

generated by pr-review-commit missing_error_handling

@@ -301,7 +300,11 @@ export function createChatGenerationContext(
if (pattern)
appendChild(

Choose a reason for hiding this comment

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

Missing error handling for potential failure of the arrayify function.

generated by pr-review-commit missing_error_handling

@@ -160,7 +160,7 @@ index N in the above snippets, and then be prefixed with exactly the same whites
the original snippets above. See also the following examples of the expected response format.

CHANGELOG:
\`\`\`
\`\`\`changelog

Choose a reason for hiding this comment

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

The changelog format has been changed from a generic code block to a specific 'changelog' code block, which may not be recognized by the rendering engine or may alter the intended presentation.

generated by pr-docs-review-commit invalid_changelog_format


## Full source ([GitHub](https://github.com/microsoft/genaiscript/blob/main/packages/vscode/genaisrc/cmt.genai.mts))

<Code code={source} wrap={true} lang="ts" title="cmt.genai.mts" />

Choose a reason for hiding this comment

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

A new file 'cmt.mdx' has been added. Ensure that all new content is accurate, all links are working, and it is correctly integrated into the documentation structure.

generated by pr-docs-review-commit new_file

@@ -34,7 +34,7 @@ See [configuration](/genaiscript/getting-started/configuration).

## Files

`run` takes one or more [glob](https://en.wikipedia.org/wiki/Glob_(programming)) patterns to match files in the workspace.
`run` takes one or more [glob](<https://en.wikipedia.org/wiki/Glob_(programming)>) patterns to match files in the workspace.

Choose a reason for hiding this comment

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

Incorrect link format, angle brackets should not be used in markdown links.

generated by pr-docs-review-commit link_format

@@ -150,7 +150,7 @@ permissions:
pull-requests: write
```

- set the `GITHUB_TOKEN` secret in the `env` when running the cli
- set the `GITHUB_TOKEN` secret in the `env` when running the cli

Choose a reason for hiding this comment

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

Incorrect indentation, there should be no leading spaces before the bullet point.

generated by pr-docs-review-commit indentation

@@ -160,7 +160,7 @@ index N in the above snippets, and then be prefixed with exactly the same whites
the original snippets above. See also the following examples of the expected response format.

CHANGELOG:
\`\`\`
\`\`\`changelog

Choose a reason for hiding this comment

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

Missing language specifier for the code fence, it should specify the language for syntax highlighting.

generated by pr-docs-review-commit code_fence_lang

import { Code } from "@astrojs/starlight/components"
import source from "../../../../../../../packages/vscode/genaisrc/cmt.genai.mts?raw"

As developers, we understand the importance of well-commented code.

Choose a reason for hiding this comment

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

Missing URL for the hyperlink in the text "Inspired by [a tweet]".

generated by pr-docs-review-commit missing_link

@pelikhan pelikhan merged commit 938abb7 into main Sep 24, 2024
8 of 10 checks passed
@pelikhan pelikhan deleted the documentor branch September 24, 2024 14:26
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