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

Don't use System.out in the ProgressListeners #2495

Merged
merged 7 commits into from
Mar 24, 2020
Merged

Don't use System.out in the ProgressListeners #2495

merged 7 commits into from
Mar 24, 2020

Conversation

BraisGabin
Copy link
Member

@BraisGabin BraisGabin commented Mar 22, 2020 •
edited
Loading

This PR is not ready because I have a problem here. The DetektProgessListener is loaded by the ServiceLoader so I can't add the PrintStream as a constructor parameter. I need to get it later using init(Config) or init(SetupContext). But neigther of them has that reference.

Should I add it to any of them? Or should I create a new init that provides both PrintStream?

This PR is part of a serie of PRs related with how we manage the output.

The main ideas:

  • Always use PrinterStream.println instead of println
  • Don't have a default printerStream. Because that is the same as use println.
  • Reduce the noise in out tests. If we always use PrinterStream to print output we can use NullPrinterStream or other types of PrinterStream so we don't print to the stdout when we run tests. With this we can reduce the log size related with tests about 40%.

@arturbosch
Copy link
Member

arturbosch commented Mar 23, 2020 •
edited
Loading

Lets add out and err to the SetupContext. Maybe even move the info, debug, error from ProcessingSettings functions to it. All extensions have a init(SetupContext) method.

@arturbosch arturbosch added this to the 1.8.0 milestone Mar 23, 2020
@BraisGabin
Copy link
Member Author

What do you think about info, debug and error as extension functions of SetupContext?

@BraisGabin BraisGabin marked this pull request as ready for review March 23, 2020 17:28
arturbosch
arturbosch requested changes Mar 23, 2020
View reviewed changes
Copy link
Member

@arturbosch arturbosch left a comment

Choose a reason for hiding this comment

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

I really like the direction of noise-less tests and standard output channels 👍

detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/SetupContext.kt

val outPrinter: PrintStream

val errPrinter: PrintStream
Copy link
Member

Choose a reason for hiding this comment

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

both printers will need kdoc to pass ci

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/runners/SingleRuleRunner.kt Outdated
@@ -51,7 +51,7 @@ class SingleRuleRunner(
val result = DetektFacade.create(
settings,
listOf(provider),
listOf(DetektProgressListener())
listOf(DetektProgressListener(outPrinter))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
listOf(DetektProgressListener(outPrinter))
listOf(DetektProgressListener().apply { init(settings) })

detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/DetektProgressListener.kt Outdated Show resolved Hide resolved
detekt-cli/src/main/kotlin/io/gitlab/arturbosch/detekt/cli/DetektProgressListener.kt Outdated
constructor()

constructor(outPrinter: PrintStream) {
this.outPrinter = outPrinter
Copy link
Member

Choose a reason for hiding this comment

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

Lets just use the init function instead introducing two constructors. I've made suggestions on the two usages.

...kt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/DetektProgressListener.kt Outdated
import org.jetbrains.kotlin.psi.KtFile
import java.io.PrintStream

class DetektProgressListener : FileProcessListener {
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this copy and just use the real progress listener from cli?

Copy link
Member

Choose a reason for hiding this comment

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

Hm, lets remove this whole class alltogether as the dots are not really useful for the generator :)

detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/Runner.kt Outdated
@@ -13,14 +13,14 @@ class Runner(
private val outPrinter: PrintStream,
private val errPrinter: PrintStream
) {
private val listeners = listOf(DetektProgressListener())
private val listeners = listOf(DetektProgressListener(outPrinter))
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove this listener. The generator output is no realy useful.

BraisGabin and others added 2 commits March 24, 2020 19:00
@BraisGabin @arturbosch
Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Set…
62e7d16 
…upContext.kt

Co-Authored-By: Artur Bosch <arturbosch@gmx.de>
@BraisGabin @arturbosch
Update detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Set…
07b345c 
…upContext.kt

Co-Authored-By: Artur Bosch <arturbosch@gmx.de>
@arturbosch arturbosch merged commit dce045c into detekt:master Mar 24, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arturbosch arturbosch arturbosch approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@BraisGabin @arturbosch