-
-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sketch of a more invasive CompletableFuture patch #126
sketch of a more invasive CompletableFuture patch #126
Conversation
🛠 Lift Auto-fixSome of the Lift findings in this PR can be automatically fixed. You can download and apply these changes in your local project directory of your branch to review the suggestions before committing.1 # Download the patch
curl https://lift.sonatype.com/api/patch/github.com/vorburger/ch.vorburger.exec/126.diff -o lift-autofixes.diff
# Apply the patch with git
git apply lift-autofixes.diff
# Review the changes
git diff Want it all in a single command? Open a terminal in your project's directory and copy and paste the following command: curl https://lift.sonatype.com/api/patch/github.com/vorburger/ch.vorburger.exec/126.diff | git apply Once you're satisfied, commit and push your changes in your project. Footnotes |
94b82c3
to
30e05e0
Compare
@mosesn this looks interesting! I'm in principle in favour of something like this, because I like the code simplification that this seems to bring. I would need to review it in full details though. And we're both so excited that both of our work overlaps a little bit - which is totally OK! Could I propose the following next steps in this order:
Would this sequence be OK for you? |
🤦 Sorry about the tabs, I haven't set up my emacs config properly for Java on my personal laptop and I try to avoid opening my work laptop on the weekend . . . I'll get right to that. That approach sounds good to me! One nit: how about we use spotless instead of checkstyle? Spotless has an autofixer that doesn't force me to use IntelliJ haha |
@mosesn voilà steps 1. & 2. are done, I'll go to 😴 now and this is all yours for 4! |
30e05e0
to
822cfc6
Compare
Looks like there are still a couple of bugs in it. Interestingly, I get a couple failures locally but they're different ones:
I think it's just discrepancies in the |
Are you on Mac locally? The tests on the main branch may well be broken on Mac. (The CI only covers Linux.) If so, raise a small separate PR to fix that, first? |
|
@mosesn Hi! It's been a while... 😃 Do you want to rebase this, and we can get this in? |
Hey, I think I'll get some time in the next few weeks to take a stab at rebasing this! Sorry for the delay |
@mosesn did you still want to rebase and clean this up for re-review and possible inclusion? Or forget about & close this one? |
@mosesn ping? 👋🏽 |
25f6841
to
381b232
Compare
Sorry for the delay! Finally found some time to work on OSS stuff while flying 🙃 |
@vorburger ready for your review when you get a chance! |
Because upcoming #126 removes AtomicExecuteResultHandler and LoggingExecuteResultHandler, which were public API, it seems appropriate to increase the version.
Because upcoming #126 removes AtomicExecuteResultHandler and LoggingExecuteResultHandler, which were public API, it seems appropriate to increase the version.
LGTM! Thank You so much for having gotten back to this (it's been a while). Merging ASAP! |
This more invasive approach (compared to #121) lets us delete a lot of code. I think it's a matter of taste whether it's better or not, but I figured this would illustrate what I meant more clearly than chatting over PRs :).
I do not think we should merge this as-is. In particular, I put a lot of logic in an anonymous lambda in the constructor, and we can likely find a better home for it. I would also completely understand you saying that you don't like this approach and simply closing the PR.