sketch of a more invasive CompletableFuture patch

[mosesn]

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.

[sonatype-lift]

FutureReturnValueIgnored: Return value of methods returning Future must be checked. Ignoring returned Futures suppresses exceptions thrown from the code that completes the Future.

---

ℹ️ Expand to see all @sonatype-lift commands

You can reply with the following commands. For example, reply with @sonatype-lift ignoreall to leave out all findings.

Command	Usage
@sonatype-lift ignore	Leave out the above finding from this PR
@sonatype-lift ignoreall	Leave out all the existing findings from this PR
@sonatype-lift exclude <file|issue|path|tool>	Exclude specified file|issue|path|tool from Lift findings by updating your config.toml file

Note: When talking to LiftBot, you need to refresh the page to see its response.
_{Click here to add LiftBot to another repo.}

[sonatype-lift]

🛠 Lift Auto-fix

Some 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.¹

# 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

1. You can preview the patch by opening the patch URL in the browser. ↩

[vorburger]

@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:

1. Do you want to review my Throw IllegalStateException instead of silently returning (wrong) Optional.empty() #123 + Catch more specific exception types, and add test coverage #124 and I merge those, for now? I do understand that this PR will then remove that again - and that's totally fine with me! I just prefer getting things merged in order, and then change them again (and have a baseline to revert back to, in the unlikely event that we would have to back this out again later).

2. I finish a PR that I have WIP to add Checkstyle, in order to settle tabs vs spaces 🤣

3. [I go to sleep]

4. You rebase this PR

5. I review this PR in details, may ask some questions and feedback, and then merge it

Would this sequence be OK for you?

[mosesn]

🤦 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

[vorburger]

@mosesn voilà steps 1. & 2. are done, I'll go to 😴 now and this is all yours for 4!

[mosesn]

Looks like there are still a couple of bugs in it. Interestingly, I get a couple failures locally but they're different ones:

[ERROR] Failures:
[ERROR]   ManagedProcessTest.onProcessFailedInvokedOnCustomListenerWithExitValueChecker:67 expected:<2> but was:<1>
[ERROR] Errors:
[ERROR]   ManagedProcessTest.mustTerminateExec:210 » ManagedProcess Program [sleep, 30s] (in working directory /Users/moses/projects/ch.vorburger.exec/.) failed with Exception: , last 100 lines of console:
usage: sleep seconds

I think it's just discrepancies in the sleep and ls implementations though. I'll have to do some poking to figure out what's going on with the other issues. I don't think I'll get to it tonight, I'll try to figure it out later this week.

[vorburger]

I think it's just discrepancies in the sleep and ls implementations though.

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]

I think it's just discrepancies in the sleep and ls implementations though.

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?

#135

[vorburger]

@mosesn Hi! It's been a while... 😃 Do you want to rebase this, and we can get this in?

[mosesn]

Hey, I think I'll get some time in the next few weeks to take a stab at rebasing this! Sorry for the delay

[vorburger]

@mosesn did you still want to rebase and clean this up for re-review and possible inclusion? Or forget about & close this one?