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

file format eval #780

Merged
merged 6 commits into from
Oct 15, 2024
Merged

file format eval #780

merged 6 commits into from
Oct 15, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Oct 15, 2024

This pull request enhances the documentation for the DIFF format, clarifying its usage for large files with minimal changes. It introduces a new file edit tool that allows for efficient updates to files, emphasizing the importance of specifying line ranges for large files. Additionally, the implementation enforces line limits for diffs and includes a new system for handling diff formats. The refactoring also disables the previous diff system and format option to streamline the process. Overall, these changes aim to improve the user experience when working with file edits and diffs.


Here is a summary of the changes reflected in the provided GIT_DIFF:

  • The DIFF file format instructions has been modified to clarify that it should be used to generate diff changes on large files with a small number of changes. This involves the specification that not more than 2 unmodified lines should be emitted before and after changes.
  • Instructions have been added on when to choose between using the DIFF file format and the FILE file format. For large files with small changes, you should use the DIFF format, else the FILE file format should be preferred.
  • The FILE file format is now to be primarily used when generating or updating files, and detailed examples have been added.
  • In the system files, a condition was added to push 'system.files' if a regex test for 'file' in jsSource returns true.
  • A new file edits_tool.genai.mts has been created with functions and tools for file editing.
  • Direct changes were made to some existing files; fib.cpp file was deleted while fib.ts file was newly added.
  • Few modifications were done to system.diff.genai.js, system.files.genai.mjs, promptdom.ts and systems.ts files.

Remember, when writing or generating new files, you should specify a start_line and end_line to update a specific portion of a large file. Don't forget to use precise delimiters to guard both your code and markdown sections. Have fun coding! 🚀 🌟

generated by pr-describe

@@ -608,7 +608,7 @@ The DIFF format should be used to generate diff changes on large files:
- only emit a couple unmodified lines before and after the changes
- keep the diffs AS SMALL AS POSSIBLE
- when reading files, ask for line numbers
- minimize the number of unmodified lines
- minimize the number of unmodified lines. DO NOT EMIT MORE THEN 2 UNMODIFIED LINES BEFORE AND AFTER THE CHANGES. Otherwise use the FILE file format.

Choose a reason for hiding this comment

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

The phrase "DO NOT EMIT MORE THEN 2 UNMODIFIED LINES" should be "DO NOT EMIT MORE THAN 2 UNMODIFIED LINES" to correct the grammatical error.

generated by pr-docs-review-commit wording

- [original line number] <deleted line>
[original line number] <2 lines after changes (not the whole file)>
[original line number] line after changes
\`\`\`

Choose a reason for hiding this comment

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

The examples provided for the DIFF format are incorrect. They should include the actual line numbers and not placeholders like "[original line number]". The examples should be updated to reflect the actual usage of the DIFF format.

generated by pr-docs-review-commit example_format

\`\`\`

`

$`- Make sure to use precisely \`\`\` to guard file code sections.
- Always sure to use precisely \`\`\`\`\` to guard file markdown sections.

Choose a reason for hiding this comment

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

The instructions for guarding file code sections are inconsistent. The first bullet point says to use precisely "```" while the second says to use precisely "````". This should be clarified for consistency.

generated by pr-docs-review-commit consistency

$`- Make sure to use precisely \`\`\` to guard file code sections.
- Always sure to use precisely \`\`\`\`\` to guard file markdown sections.
- Use full path of filename in code section header.`
- Use full path of filename in code section header.
- Use start_line, end_line for large files with small updates`

Choose a reason for hiding this comment

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

The phrase "Always sure to use precisely" is grammatically incorrect. It should be "Always be sure to use precisely" to correct the sentence structure.

generated by pr-docs-review-commit wording

@@ -243,12 +243,14 @@ function renderDefNode(def: PromptDefNode): string {
while (dfence && body.includes(dfence)) {
dfence += "`"
}
const diffFormat = "" // body.length > 500 ? "preferred_diff_format=DIFF" : ""

Choose a reason for hiding this comment

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

Unused variable 'diffFormat'. Consider removing it if it's not used.

generated by pr-review-commit unused_variable

@@ -45,6 +45,7 @@ export function resolveSystems(
if (/\Wchangelog\W/i.test(jsSource)) systems.push("system.changelog")
else if (/\Wfile\W/i.test(jsSource)) {
systems.push("system.files")
// systems.push("system.diff")

Choose a reason for hiding this comment

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

There is a commented out code. If it's not needed, consider removing it.

generated by pr-review-commit commented_code

"A gentle touch, a heart's warmth.",
]
function fibonacci(n: number): number {
// TODO: implement fibonacci algorithm

Choose a reason for hiding this comment

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

There is a TODO comment in the code, consider completing the implementation or creating a task to track this TODO.

generated by pr-review-commit todo_in_code

Copy link

The changes presented in the GIT_DIFF involve three files:

  1. packages/core/src/promptdom.ts: A new variable called diffFormat was added but it's not being used anywhere. It's unclear what its purpose is.

  2. packages/core/src/systems.ts: A commented line of code was added, potentially indicating future development plans.

  3. packages/sample/src/edits/su/fib.ts: This file is new and mostly contains an array of strings. A fibonacci function was created but is not fully implemented and always returns 0.

Here are the concerns about these changes:

  • The diffFormat variable in promptdom.ts is declared but not being used.
  • The commented line in systems.ts doesn't provide any value and may be confusing.
  • The fibonacci function in fib.ts is not implemented.

Suggested changes are as follows:

diff --git a/packages/core/src/promptdom.ts b/packages/core/src/promptdom.ts
--- a/packages/core/src/promptdom.ts
+++ b/packages/core/src/promptdom.ts
@@ -243,14 +243,12 @@ function renderDefNode(def: PromptDefNode): string {
-    const diffFormat = "" // body.length > 500 ? "preferred_diff_format=DIFF" : ""
...

diff --git a/packages/core/src/systems.ts b/packages/core/src/systems.ts
--- a/packages/core/src/systems.ts
+++ b/packages/core/src/systems.ts
@@ -45,7 +45,6 @@ export function resolveSystems(
-           // systems.push("system.diff")

diff --git a/packages/sample/src/edits/su/fib.ts b/packages/sample/src/edits/su/fib.ts
--- a/packages/sample/src/edits/su/fib.ts
+++ b/packages/sample/src/edits/su/fib.ts
@@ -211,7 +211,7 @@ function fibonacci(n: number): number {
-    return 0 // BODY
+    // TODO: Implement the function
}

These changes will remove unused code and make it clear that the fibonacci function needs to be implemented, reducing potential confusion.

generated by pr-review

@@ -608,7 +608,7 @@ The DIFF format should be used to generate diff changes on large files:
- only emit a couple unmodified lines before and after the changes
- keep the diffs AS SMALL AS POSSIBLE
- when reading files, ask for line numbers
- minimize the number of unmodified lines
- minimize the number of unmodified lines. DO NOT EMIT MORE THEN 2 UNMODIFIED LINES BEFORE AND AFTER THE CHANGES. Otherwise use the FILE file format.

Choose a reason for hiding this comment

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

Typo in the documentation, "THEN" should be "THAN".

generated by pr-docs-review-commit typo

@@ -623,36 +623,40 @@ The DIFF format should be used to generate diff changes on large files:
FOLLOW THE SYNTAX PRECISLY. THIS IS IMPORTANT.

Choose a reason for hiding this comment

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

Typo in the documentation, "PRECISLY" should be "PRECISELY".

generated by pr-docs-review-commit typo

\`\`\`

`

$`- Make sure to use precisely \`\`\` to guard file code sections.
- Always sure to use precisely \`\`\`\`\` to guard file markdown sections.

Choose a reason for hiding this comment

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

Typo in the documentation, "Always sure" should be "Always be sure".

generated by pr-docs-review-commit typo

\`\`\`

`

$`- Make sure to use precisely \`\`\` to guard file code sections.
- Always sure to use precisely \`\`\`\`\` to guard file markdown sections.

Choose a reason for hiding this comment

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

Typo in the documentation, "guard file markdown sections" should be "guard markdown file sections".

generated by pr-docs-review-commit typo

const res = parseChangeLogs(source)
console.log(res)
assert.equal(res[0].filename, "src/edits/su/fib.ts")
})

Choose a reason for hiding this comment

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

The test case "missing header" does not contain any assertions. This could lead to false positives where the test passes but the functionality is not working as expected.

generated by pr-review-commit missing_test_assertion

const res =
(name ? name + ":\n" : "") +
dfence +
dtype +
(file.filename ? ` file="${file.filename}"` : "") +
(schema ? ` schema=${schema}` : "") +
diffFormat +

Choose a reason for hiding this comment

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

The variable 'diffFormat' is declared but its value is never read. Consider removing it if it's not needed.

generated by pr-review-commit unused_variable

@pelikhan pelikhan merged commit b4c6270 into main Oct 15, 2024
8 of 10 checks passed
@pelikhan pelikhan deleted the file-formats branch October 15, 2024 22:50
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