search

pwntester / octo.nvim

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

Fix thread header becoming invisible after toggling resolved-state #464

Closed Daniel1854 closed 9 months ago

Daniel1854 commented 9 months ago

Describe what this PR does / why we need it

When resolving threads via Octo thread [un]resolve the virtual text of the thread should be updated with the new resolved-state. Currently the text only gets cleared due to an oversight.

Does this pull request fix one issue?

Fixes #362

Describe how you did it

Added a field, which was needed since a feature commit

Describe how to verify it

Tested it within the timeline (Octo pr list) aswell as within the review mode (Octo review start). Just resolved and unresolved some threads.

Special notes for reviews

Fwiw this seems to be introduced with the commit 50704c5071e9dd4a5643f55403f146b2bbe710bc. With this commit the wrapper around writing this specific virtual_text now expects a commit field.

I have added a for loop to gather the commit sha of the actual thread from the response from github. If there is an easier way, Im happy to change it.

Sidenote: the duplicated code between resolve_thread and unresolve_thread increases with this commit. If you want, I can extract those twenty-ish lines into another function

pwntester commented 9 months ago

Thanks for the fix! Yep, I think it will be better to move the shared code to its own function. Can you please do that?

Daniel1854 commented 9 months ago

Sure, I extracted the logic. Let me know if there is anything else to fix

pwntester commented 9 months ago

Nice! Thanks a lot!