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.

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

Add printers to the command line runner #2188

Merged
merged 4 commits into from
Dec 22, 2019
Merged

Add printers to the command line runner #2188

merged 4 commits into from
Dec 22, 2019

Conversation

arturdryomov
Copy link
Contributor

A bit of context to understand the motivation behind the change.

Me and @artem-zinnatullin are working on the Bazel rule for Detekt. One of the features of the rule is the persistent workers support.

  • The thing with workers is that standard inputs and outputs are reserved for the Bazel / worker communication. Since there is no API to limit Detekt standard output usage we were forced to create a workaround which replaces system standard output streams with file-based ones.
  • This worked fine (though a bit hacky) until we thought about multiplexing. The multiplexing will change the flow, essentially we’ll be calling multiple Detekt runners in parallel. Unfortunately, the replace-system-streams workaround will not work in a multi-thread environment since it will be almost impossible to understand which runner printed which output to a singleton system stream.

If this change is accepted (and eventually published) we’ll be able to make Detekt invocations more or less hermetic. At the same time, the API change is backwards-compatible (arguments fall back to default values) and kinda makes sense abstracting from our personal needs (there is no way to change printers since ProcessingSettings in Runner are not accessible).

@codecov-io
Copy link

codecov-io commented Dec 20, 2019

Codecov Report

Merging #2188 into master will increase coverage by 0.36%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2188      +/-   ##
============================================
+ Coverage     80.75%   81.12%   +0.36%     
- Complexity     2032     2055      +23     
============================================
  Files           338      339       +1     
  Lines          5841     5874      +33     
  Branches       1066     1070       +4     
============================================
+ Hits           4717     4765      +48     
+ Misses          554      536      -18     
- Partials        570      573       +3
Impacted Files Coverage Δ Complexity Δ
...ain/kotlin/io/gitlab/arturbosch/detekt/cli/Main.kt 50% <100%> (+50%) 7 <1> (+7) ⬆️
.../io/gitlab/arturbosch/detekt/cli/runners/Runner.kt 88.88% <100%> (+1.08%) 10 <2> (+1) ⬆️
...b/arturbosch/detekt/rules/style/ForbiddenImport.kt 95% <0%> (-0.66%) 9% <0%> (ø)
...bosch/detekt/rules/providers/StyleGuideProvider.kt 100% <0%> (ø) 3% <0%> (ø) ⬇️
...sch/detekt/rules/style/ForbiddenPublicDataClass.kt 90.47% <0%> (ø) 13% <0%> (?)
...gitlab/arturbosch/detekt/cli/runners/AstPrinter.kt 77.14% <0%> (+2.85%) 1% <0%> (+1%) ⬆️
...in/io/gitlab/arturbosch/detekt/api/SplitPattern.kt 85.18% <0%> (+6.23%) 17% <0%> (ø) ⬇️
.../kotlin/io/gitlab/arturbosch/detekt/cli/CliArgs.kt 100% <0%> (+7.4%) 6% <0%> (ø) ⬇️
...ab/arturbosch/detekt/cli/runners/ConfigExporter.kt 20% <0%> (+20%) 1% <0%> (+1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 62b8680...89f2060. Read the comment docs.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Fine for me, since this PR is backwards compatible.

@schalkms
Copy link
Member

@arturdryomov the bazel detekt plugin is cool. Thanks for doing this! +1 star

@arturdryomov
Copy link
Contributor Author

arturdryomov commented Dec 21, 2019

@schalkms, sure — expanded the test suite a bit.

Copy link
Member

@schalkms schalkms left a comment

Choose a reason for hiding this comment

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

Thanks! 🥇

@arturbosch arturbosch added this to the 1.3.0 milestone Dec 22, 2019
@arturbosch arturbosch merged commit e3c59a1 into detekt:master Dec 22, 2019
@arturdryomov arturdryomov deleted the ad/runner-printers branch December 22, 2019 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants