git-machete-intellij-plugin
git-machete-intellij-plugin copied to clipboard
VirtusLab
Traverse feature
As a general remark, pls push code that's already formatted (should happen automatically on compilation), then the plugin zip will be readily available for download from the CI (ofc assuming other checks passed, but I assume they do here, other than formatting) 😅
ping @zmerr (in case GitHub didn't send you a notification after my review 😉 )
apart from the issues with lombok and failing CI...
I see you have extracted override fork point, pull, reset to remote and sync to parent by rebse backgroundables 🔧 As it is an extensive refactor please do not mix it with the new feature 🙅 Let's introduce these changes on a separate PR before this one 👈 ok? 🙂
I general I do admit - it's a good idea to extract the backgroundables 🚀 it's a code quality improvement. As I can see for now (TraverseAction.java) it will be very handy here 👍
Thanks for reviewing this. I agree with putting the refactoring in a separate PR. still, I need to put them all together in the traverse through overriding their onSuccess and onFinish methods to make sure the right thing gets refactored and can be used in Traverse.
After that, I would create a separate PR for them.
It seems that currently most of the push functionality is interwoven within the code of GitPushDialog.
I guess for making it work, this code has to get refactored inside the BasePushAction first, such that the GitPushDialog only contains the presentation specific code.
It seems that currently most of the push functionality is interwoven within the code of
GitPushDialog. I guess for making it work, this code has to get refactored inside theBasePushActionfirst, such that theGitPushDialogonly contains the presentation specific code.
It is very likely that GitPushDialog is a mess (I will not point out who the main author of this class is 👈 ) 😆 If you are willing to do the refactor I strongly encourage you to do it on a separate PR - both implementation and review will be much easier! 🙂
In spite of the issue with mentioned GitPushDialog is there anything blocking you now or that you need help with?
It seems that currently most of the push functionality is interwoven within the code of
GitPushDialog. I guess for making it work, this code has to get refactored inside theBasePushActionfirst, such that theGitPushDialogonly contains the presentation specific code.It is very likely that
GitPushDialogis a mess (I will not point out who the main author of this class is 👈 ) 😆 If you are willing to do the refactor I strongly encourage you to do it on a separate PR - both implementation and review will be much easier! 🙂
No problem, I would be refactoring it in a new PR 🙂
In spite of the issue with mentioned
GitPushDialogis there anything blocking you now or that you need help with?
No nothing else. As soon as the GitPushDialog gets refactored all things can go ahead smoothly.
Right now, the syncToRemote is correctly triggered onSuccess of syncToParent
What is left is triggering checkout and traverse of the next branch onSuccess of syncToRemote, which would be possible after sorting out the GitPushDialog
In spite of the issue with mentioned
GitPushDialogis there anything blocking you now or that you need help with?No nothing else. As soon as the
GitPushDialoggets refactored all things can go ahead smoothly.Right now, the
syncToRemoteis correctly triggeredonSuccessofsyncToParentWhat is left is triggering checkout and
traverseof the next branchonSuccessofsyncToRemote, which would be possible after sorting out theGitPushDialog
Great 🚀 I agree with the idea of refactor of both GitPushDialog and the backgroundables ✅ and it’s good to see we are that close to the traverse feature 🌟
Kudos for your help with this one and fingers crossed 🤞🏻 you are doing a good job 💪🏻
Hey @zmerr, let me take the traverse feature from here (we need to speed up things a bit 🤣 ). I ve created a new PR (https://github.com/VirtusLab/git-machete-intellij-plugin/pull/1352) that takes the most recent changes into account. I am still open for your input/feedback/review ofc - just to make the point clear - with me as an author and owner of this one now. I hope you have no problem with that 🙂
Having the above ☝️ I am closing this one (but you can be sure that most of your changes will not be abandoned 🗑️ ! they are already in the new PR and thank you for that work 🚀 ).
Hey @zmerr, let me take the traverse feature from here (we need to speed up things a bit 🤣 ). I ve created a new PR (#1352) that takes the most recent changes into account. I am still open for your input/feedback/review ofc - just to make the point clear - with me as an author and owner of this one now. I hope you have no problem with that 🙂
Having the above ☝️ I am closing this one (but you can be sure that most of your changes will not be abandoned 🗑️ ! they are already in the new PR and thank you for that work 🚀 ).
Hi @mkondratek, I really appreciate that you picked this up and included the good part of the changes😊 You’re doing an amazing job 🚀