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

split docs/produtct dependencies #621

Merged
merged 14 commits into from
Aug 16, 2024
Merged

split docs/produtct dependencies #621

merged 14 commits into from
Aug 16, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 15, 2024

Split workspace to avoid docs/slides pollute genaiscript dependency list

  • 🚀 A new script command install:force has been added to the package.json for forcefully reinstalling dependencies.
  • 💅 The astro version in dependencies has been upgraded from ^4.13.1 to ^4.14.2.
  • 🎉 Several other dependency packages have been upgraded including @astrojs/check, @astrojs/starlight, and typescript.
  • 🔧 In commands.md, the default promptfoo version has been updated from ^0.75.2 to 0.78.0.
  • 📝 Some documentation updates are made in files.md and parsers.md to include information about readXML.
  • 🆕 A new file - yarn.lock - has been introduced into the project.
  • ➕ The addition of several new dependencies in devDependencies.
  • 👌 Tweaked the Cli reference documentation, more specifically the syntax of fence, html-to-text, and their parse commands in commands.md.
  • 📚 Content updates on files.md and parsers.md, adding new methods and examples associated with parsing XML content.

generated by pr-describe

@pelikhan
Copy link
Member Author

/genai-describe


```ts
const data = await workspace.readXML("data.xml")
```

Choose a reason for hiding this comment

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

The readXML function is added without a description of its behavior or use cases.

generated by pr-docs-review-commit missing_example_description

"@_attr": "1",
"child": {}
}
}

Choose a reason for hiding this comment

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

The code fence around the JSON example should be js or javascript, not ```json, since the content is not valid JSON due to the unquoted keys.

generated by pr-docs-review-commit incorrect_code_fence

const file = await fs.readText(f)
const res = XMLParse(file.content)
return res
},

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 XML parsing. If the XML is malformed, this could cause an unhandled exception.

generated by pr-review-commit missing_error_handling

@@ -90,6 +90,7 @@ export async function createPromptContext(
const workspace: WorkspaceFileSystem = {
readText: (f) => runtimeHost.workspace.readText(f),
readJSON: (f) => runtimeHost.workspace.readJSON(f),
readXML: (f) => runtimeHost.workspace.readXML(f),

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 XML reading. If the XML is malformed or the file does not exist, this could cause an unhandled exception.

generated by pr-review-commit missing_error_handling

ignoreDeclaration: true,
parseAttributeValue: true,
...(options || {}),
})

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 XML parsing. If the XML is malformed, this could cause an unhandled exception.

generated by pr-review-commit missing_error_handling

ignoreDeclaration: true,
parseAttributeValue: true,
...(options || {}),
})

Choose a reason for hiding this comment

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

The attribute name prefix is hardcoded to "@_". This could lead to confusion or unexpected behavior if other parts of the codebase expect a different prefix. Consider making this a configurable option. 🔄

generated by pr-review-commit hardcoded_values

test("parse attribute", () => {
const x = XMLParse('<root a="1"><b>2</b></root>')
assert.deepStrictEqual(x, { root: { b: 2, "@_a": 1 } })

Choose a reason for hiding this comment

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

The test case for XML parsing only checks for a single attribute. It would be more robust to include multiple attributes in the test case to ensure the parser can handle them correctly. 🧪

generated by pr-review-commit incomplete_test


```ts
const data = await workspace.readXML("data.xml")
```

Choose a reason for hiding this comment

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

The readXML function is added but there is no explanation or details provided about its usage or behavior.

generated by pr-docs-review-commit missing_documentation

"@_attr": "1",
"child": {}
}
}

Choose a reason for hiding this comment

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

The example provided for parsers.XML function is in JSON format, which is inconsistent with the context that should show an XML parsing example.

generated by pr-docs-review-commit incorrect_example

@@ -87,7 +87,7 @@ Options:
-td, --test-delay <string> delay between tests in seconds

Choose a reason for hiding this comment

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

The option name --promptfoo-version seems incorrect. It should likely be --prompt-version.

generated by pr-docs-review-commit option_name


```ts
const data = await workspace.readXML("data.xml")
```

Choose a reason for hiding this comment

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

A new readXML function has been documented. Ensure that the implementation matches the documentation.

generated by pr-docs-review-commit new_feature

"@_attr": "1",
"child": {}
}
}

Choose a reason for hiding this comment

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

The parsers.XML function documentation has been updated with more details on how attributes are parsed. Ensure that the implementation matches the documentation.

generated by pr-docs-review-commit xml_parsing_details

@@ -249,7 +249,7 @@ export async function cli() {
.alias("parsers")
.description("Parse various outputs")
parser
.command("fence <language>")
.command("fence <language> <file>")

Choose a reason for hiding this comment

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

The parseFence function now requires two arguments, but only one is being passed here. Please pass the required file argument. 📝

generated by pr-review-commit missing_argument

const fences = extractFenced(stdin || "").filter(
export async function parseFence(language: string, file: string) {
const res = await parsePdf(file)
const fences = extractFenced(res.content || "").filter(

Choose a reason for hiding this comment

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

The function parsePdf is not defined in this file or imported from another module. Please ensure that this function is defined before it is called. 🧐

generated by pr-review-commit undefined_function

@@ -90,6 +90,7 @@ export async function createPromptContext(
const workspace: WorkspaceFileSystem = {
readText: (f) => runtimeHost.workspace.readText(f),
readJSON: (f) => runtimeHost.workspace.readJSON(f),
readXML: (f) => runtimeHost.workspace.readXML(f),

Choose a reason for hiding this comment

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

The readXML function is not defined in the runtimeHost.workspace object. Please ensure that this function is defined before it is called. 🕵️‍♀️

generated by pr-review-commit missing_function

@@ -292,10 +292,10 @@ Options:
-h, --help display help for command

Commands:

Choose a reason for hiding this comment

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

The command usage for fence is missing the <file> argument.

generated by pr-docs-review-commit command_usage

pdf <file> Parse a PDF into text
docx <file> Parse a DOCX into texts
html-to-text [file] Parse an HTML file into text
html-to-text <file> Parse an HTML file into text

Choose a reason for hiding this comment

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

The command usage for html-to-text is missing the <file> argument.

generated by pr-docs-review-commit command_usage

@@ -305,7 +305,7 @@ Commands:
### `parse fence`

```
Usage: genaiscript parse fence [options] <language>
Usage: genaiscript parse fence [options] <language> <file>

Choose a reason for hiding this comment

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

The command usage for parse fence is missing the <file> argument.

generated by pr-docs-review-commit command_usage

@@ -338,7 +338,7 @@ Options:
### `parse html-to-text`

```
Usage: genaiscript parse html-to-text [options] [file]
Usage: genaiscript parse html-to-text [options] <file>

Choose a reason for hiding this comment

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

The command usage for parse html-to-text is missing the <file> argument.

generated by pr-docs-review-commit command_usage

"@_attr": "1",
"child": {}
}
}

Choose a reason for hiding this comment

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

The parsers.XML function documentation has been updated with new information about attribute naming conventions. Ensure that the implementation matches the documentation.

generated by pr-docs-review-commit new_feature

const fences = extractFenced(stdin || "").filter(
export async function parseFence(language: string, file: string) {
const res = await parsePdf(file)
const fences = extractFenced(res.content || "").filter(

Choose a reason for hiding this comment

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

The function parsePdf is called here, but it is not defined or imported in this file. Please check the function name or import it.

generated by pr-review-commit wrong_function_call

allowBooleanAttributes: true,
ignoreDeclaration: true,
parseAttributeValue: true,
...(options || {}),

Choose a reason for hiding this comment

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

The options object is being overwritten here, which could lead to unexpected behavior if options are passed to the XMLParse function. Consider merging the default options with the passed options instead of overwriting them.

generated by pr-review-commit unnecessary_code

allowBooleanAttributes: true,
ignoreDeclaration: true,
parseAttributeValue: true,
...(options || {}),

Choose a reason for hiding this comment

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

The XML parser is now configured to parse attributes, which might lead to unexpected behavior if the rest of the codebase is not prepared to handle attributes.

generated by pr-review-commit unexpected_behavior

@pelikhan pelikhan changed the title ++Upgrade dependencies split docs/produtct dependencies Aug 16, 2024
@@ -264,7 +264,7 @@ export async function cli() {
.action(parseDOCX)

parser
.command("html-to-text [file]")
.command("html-to-text <file>")

Choose a reason for hiding this comment

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

The function parseHTMLToText is now expecting one argument, but none is being provided. Please provide the required argument.

generated by pr-review-commit missing_argument

@pelikhan
Copy link
Member Author

/genai-describe

allowBooleanAttributes: true,
ignoreDeclaration: true,
parseAttributeValue: true,
...(options || {}),

Choose a reason for hiding this comment

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

The XML parser options have been changed. This could potentially break existing functionality if the parser is used elsewhere in the codebase.

generated by pr-review-commit unexpected_change

@pelikhan pelikhan merged commit c9f9795 into main Aug 16, 2024
10 checks passed
@pelikhan pelikhan deleted the dep081524 branch August 16, 2024 15:21
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