pwntester / octo.nvim

Edit and review GitHub issues and pull requests from the comfort of your favorite editor
MIT License
2.28k stars 125 forks source link

Allow user to configure if they should use local file system for review #349

Closed NWVi closed 1 year ago

NWVi commented 1 year ago

Describe what this PR does / why we need it

Allow users to configure if they want to use local fs when doing a review.

Does this pull request fix one issue?

No, but addresses comments in discussion https://github.com/pwntester/octo.nvim/discussions/225#discussioncomment-4177857

Describe how you did it

Add field to configuration and use it to determine if local fs should be used. If use_localfs is true and user initiates a review at a branch other than the PR branch, ask if we should checkout to PR branch. Most of the code had previously been commented out, but seemed to work assuming the review branch is checked out.

Describe how to verify it

Set use_localfs = true and check path of right pane. If used from branch other then PR branch you have to confirm checking out branch.

Special notes for reviews

I made a synchronous version of the checkout util since I had to wait for swapping to PR branch before proceeding to Review layout. If you have suggestions for how I can avoid doing this it would be appreciated.

pwntester commented 1 year ago

Thanks for the PR! It looks good to me, I think that warning the user about the branch change should be enough so we can let him revert changes on his own. Need to check if we need to change branches somewhere else such as when reviewing at commit level and changing to a new commit

UPDATE: Yep, seems like we should also be changing branches at Review:focus_commit(right, left)

NWVi commented 1 year ago

I renamed use_localfs to use_local_fs. šŸ‘

I'm a little uncertain about the changing brances at Review:focus_commit(right, left). From what I could tell that is only called when using the Octo review commit command, and I assume you at that point you have decided to checkout a branch or not at that point. Are you thinking that we chould be checking out the right commit?

carlosflorencio commented 1 year ago

Is anything missing to have this merged? šŸ™

delabere commented 1 year ago

Was testing this a little today and has been working nicely for me so far - thank you!

NWVi commented 1 year ago

Is anything missing to have this merged? šŸ™

There is one part still remaining before this is ready to me merged, which is to checkout the different commits when doing :Octo review commit. I paused my work on the PR over the holidays, but will try to finish it soon. šŸ˜…

SChakravorti21 commented 1 year ago

I was testing this out a bit today and it was super nice having LSP for navigating complex changes, seeing linter warnings, etc. @NWVi thank you for implementing this!

sf8193 commented 1 year ago

works for me, very eager to get this functionality

lazywei commented 1 year ago

friendly ping @pwntester do we plan to merge this anytime soon :) Thanks!

pwntester commented 1 year ago

I think, it should be tested enough so merging it. If you find any bugs please report