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

fix: do not fail if only removing lines from file #174

Merged
merged 1 commit into from
Aug 14, 2023

Conversation

gowoons
Copy link
Collaborator

@gowoons gowoons commented Aug 14, 2023

There is a bug where if a file only has had lines removed, the review will not work
This should solve it

@github-actions
Copy link
Contributor

Risk Level 2 - /home/runner/work/code-review-gpt/code-review-gpt/src/review/prompt/constructPrompt/batchFiles/utils/createPromptFiles.ts

The code changes seem to be well written and follow good practices. However, there are a few areas that could be improved for better readability and maintainability:

  1. Use of shift() method: In the getChangedIndicesAndLength function, you are using shift() method on the indices array. This operation can be costly as it re-indexes the array after removing the first element. If the array is large, this could impact performance. Consider using a different approach if the order of elements in the array is not important.

  2. Use of == operator: In the createPromptFiles function, you are using == operator for comparison. It's generally recommended to use === operator in JavaScript/TypeScript to avoid unexpected type coercion.

  3. Use of Infinity and -Infinity: In the getChangedIndicesAndLength function, you are initializing minIndex and maxIndex with Infinity and -Infinity respectively. While this is not wrong, it might be confusing for some developers. Consider adding a comment explaining why you are doing this.

Here are the suggested changes:

// In getChangedIndicesAndLength function
// Instead of shift, consider using a counter to track the index
let counter = 0;
// ...
const index = indices[counter++];

// In createPromptFiles function
// Use === instead of ==
if (totalChangedLinesLength === 0) {
  return result;
}

🔄🔍💬


Powered by Code Review GPT

@github-actions
Copy link
Contributor

Test results summary:

✅ [PASS] - Test case: Bad variable name
✅ [PASS] - Test case: Exposed secret
✅ [PASS] - Test case: Too many nested loops
✅ [PASS] - Test case: Unawaited Promise

SUMMARY: ✅ PASS: 4 - ⚠️ WARN: 0 - ❌ FAIL: 0


Tests Powered by Code Review GPT

@gowoons gowoons marked this pull request as draft August 14, 2023 15:39
@gowoons gowoons marked this pull request as ready for review August 14, 2023 16:04
@mattzcarey mattzcarey merged commit a33f49a into main Aug 14, 2023
2 checks passed
@mattzcarey mattzcarey deleted the fix/dont-fail-when-only-removing-lines-from-file branch August 14, 2023 16:06
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.

[Bug] AAUser, I don't have a failure when there are only lines being removed in a file
2 participants