Open jeonbyeongmin opened 1 year ago
@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
@anc95 Great! Can I modify the code myself? I wonder how I can run the test of this app.
@jeonbyeongmin Appreciated
Bad new is the it's not easy to test. There are two ways
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
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.
@anc95 I understand, but I would like to know more about the second way -- github actions How can i trigger an unpublished action?
@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
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!
@anc95
I have run numerous tests, and I have some bad news. The modified code is not complete.
Do you know how Octokit works? I'm not sure why it's not properly excluding files.
@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
@anc95 Great! However, it seems like more testing is needed. I'll go ahead and submit a PR for now. :)
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 thedata.data
object includes files with changes, including deleted files. Thefile.status
property can be one ofadded
,modified
,removed
,renamed
, orcopied
. Therefore, it is suggested to exclude the files with theremoved
andrenamed
status from thechangeFiles
. It may also be helpful to add an environment variable to control this behavior.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.