How about excluding removed and renamed 'files' from code reviews?

[jeonbyeongmin]

Hello!
I'm using your app really really well.
Let me give you a suggestion.

Current Issue:

The code review conducted by the app includes deleted files, which is not necessary. In the code file src/bot.ts, the changedFiles variable obtained from the data.data object includes files with changes, including deleted files. The file.status property can be one of added, modified, removed, renamed, or copied. Therefore, it is suggested to exclude the files with the removed and renamed status from the changeFiles. It may also be helpful to add an environment variable to control this behavior.

// src/bot.ts

let { files: changedFiles, commits } = data.data; // 68 line

Proposed Solution:

Exclude the files with the removed and renamed status from the changeFiles function, and consider adding an environment variable to control this behavior.

[anc95]

@jeonbyeongmin Yes. This a good idea. Considering removed files are not Being used anymore, and review the removed/renamed files is no help on the code, and it's good for reducing the user's token cost

Let's have excluding renamed/removed files by default, if there is any request for reviewing the renamed/removed files in the future, we can consider to add process env to support this configuration then

[jeonbyeongmin]

@anc95 Great! Can I modify the code myself? I wonder how I can run the test of this app.

[anc95]

@jeonbyeongmin Appreciated

Bad new is the it's not easy to test. There are two ways

1. Using GitHub app, the code base is based on probot, you can search and find out how to test with probot.you need to create a GitHub app and get properly configured, then setup all the local env, and install the app for one of your repo, and then trigger the event.
   It's really complicated ha

2. The second way is using the GitHub actions, you can setup a GitHub action using the action based on your working branch, then trigger action for test.

I am not sure if there is a unit test way to mock data and test.

Sorry that I can't reach my Mac now, so I just typed in an phone, it's not easier to express things clearly to you.

You can try the above two ways, any questions please kindly let me know. Thanks.

[jeonbyeongmin]

@anc95
I understand, but I would like to know more about the second way -- github actions
How can i trigger an unpublished action?

[anc95]

@jeonbyeongmin i am guessing don't need to punish an action

In my case, I am using

anc95/ChatGPT-CodeReview@dev

To test. It can be found and triggered

You can try to replace the repo owner and branch to yours to test

[jeonbyeongmin]

Ah, If I apply it to the repository I forked, it would be jeonbyeongmin/ChatGPT-CodeReview@dev, right?

---

I tested it on jeonbyeongmin/ChatGPT-CodeReview@dev and confirmed that it works properly!
I will resolve this issue and submit a PR! Thank you for explaining it kindly!

[jeonbyeongmin]

@anc95

I have run numerous tests, and I have some bad news.
The modified code is not complete.

1. "Renamed" is not excluded in any case.
2. Only when the pull_request type is "synchronize" (i.e., when commits are added after the PR is opened) are removed files excluded. They are not excluded in the "opened" state.

Do you know how Octokit works? I'm not sure why it's not properly excluding files.

[anc95]

@jeonbyeongmin Sorry for being so late to reply you
I searched the document. there is no clear introduction for files, here is the document for compare two commits. it says

The API response includes details about the files that were changed between the two commits. This includes the status of the change (if a file was added, removed, modified, or renamed), and details of the change itself. For example, files with a renamed status have a previous_filename field showing the previous filename of the file, and files with a modified status have a patch field showing the changes made to the file.

I am not sure too. but if we can exclude removed files in both open/sync event and exclude renamed files in sync event, it's already is a good improvement

[jeonbyeongmin]

@anc95
Great! However, it seems like more testing is needed. I'll go ahead and submit a PR for now. :)