Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LogOutputStream.create(LineConsumer) #107

Merged
merged 14 commits into from
Mar 15, 2024
Merged

LogOutputStream.create(LineConsumer) #107

merged 14 commits into from
Mar 15, 2024

Conversation

panchenko
Copy link
Contributor

So instead of creating an anonymous class it can be one-liner with lambda expression.

@panchenko
Copy link
Contributor Author

If we are going to proceed with this, then I can also add an example to the readme file.

nemecec
nemecec requested changes Mar 3, 2023
View reviewed changes
Copy link
Collaborator

@nemecec nemecec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool idea, thanks! I think the JavaDoc could be better (I added some comments with suggestions, feel free to use those as inspiration).
Also, a unit test and example usage in README would be much appreciated.

src/main/java/org/zeroturnaround/exec/stream/LogOutputStream.java Outdated Show resolved Hide resolved
src/main/java/org/zeroturnaround/exec/stream/LogOutputStream.java Outdated Show resolved Hide resolved
panchenko
panchenko commented Mar 4, 2023
View reviewed changes
README.md Outdated

```java
new ProcessExecutor().command("java", "-version")
.redirectOutput(LogOutputStream.create(line -> ...))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, there could be also a new method here .processOutput(line -> ...), WDYT?

@aalmiray
Copy link
Contributor

Bytecode compatibility is set to Java 6 as a minimum which will prevent the use of lambdas.
I'd suggest bumping to 7 as a minimum as JDK 17 (see PR for build) fails if 6 is chosen.
Another option would be to bump to Java 8 as a minimum, in which case lambdas may be used.

Bear in mind that bumping Java baseline constitutes a breaking change and thus the library should increase its major version number according to semver.

@panchenko
Copy link
Contributor Author

The lambdas will be not here, but in code using this library.

@panchenko
Copy link
Contributor Author

@nemecec Could you lease take a look? Thanks!

@panchenko
Copy link
Contributor Author

Is this project maintained?

@nemecec
Copy link
Collaborator

nemecec commented Mar 15, 2024

Is this project maintained?

Sorry, I totally forgot about this PR. Looks good, I'll merge it in.

@nemecec nemecec merged commit 2ba09d1 into zeroturnaround:main Mar 15, 2024
4 checks passed
@nemecec
Copy link
Collaborator

nemecec commented Mar 15, 2024

Thanks for the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nemecec nemecec nemecec requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@panchenko @aalmiray @nemecec