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

Remove xml report consolidation. #1774

Merged
merged 3 commits into from
Jul 28, 2019
Merged

Remove xml report consolidation. #1774

merged 3 commits into from
Jul 28, 2019

Conversation

marschwar
Copy link
Contributor

I realize this is going to be a bit controversial but I wanted to get the discussion started anyway.

IMHO the report consolidation as it currently stands should be removed. Reasons for that being:

I realize that this was heavily requested feature, but the way it is implemented simply introduces more problems than benefits.

Reverts #1329

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.

First of all, thank you for your efforts, Markus!
I think it's also a good idea to revert this.
I agree with you that this approach introduces more problems than benefits.
It would take another stab at this, I think.
Anyhow, I really appreciate all the work, which has been done on this feature.

@marschwar the linked PR in #1329 changed quite a lot. Are you sure that everything, which is related to this feature, actually got removed in this PR?

@marschwar
Copy link
Contributor Author

Thank you for making me take another look. While most of the unreverted changes were refactorings in test code and formatting there was one more thing that could be removed. With this the detekt task does not have to depend on the detekt task of its child projects.

@codecov-io
Copy link

codecov-io commented Jul 21, 2019

Codecov Report

Merging #1774 into master will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1774      +/-   ##
============================================
- Coverage     79.86%   79.82%   -0.05%     
+ Complexity     1939     1937       -2     
============================================
  Files           325      324       -1     
  Lines          5473     5472       -1     
  Branches       1010     1009       -1     
============================================
- Hits           4371     4368       -3     
- Misses          580      581       +1     
- Partials        522      523       +1
Impacted Files Coverage Δ Complexity Δ
...b/arturbosch/detekt/rules/empty/EmptyCatchBlock.kt 57.14% <0%> (-26.2%) 3% <0%> (ø)
...n/io/gitlab/arturbosch/detekt/rules/LinesOfCode.kt 96.55% <0%> (-3.45%) 0% <0%> (ø)
...osch/detekt/rules/exceptions/SwallowedException.kt 65.38% <0%> (-2.48%) 13% <0%> (-3%)
...tekt/rules/exceptions/TooGenericExceptionCaught.kt 86.95% <0%> (ø) 6% <0%> (+1%) ⬆️
...in/kotlin/io/gitlab/arturbosch/detekt/api/Issue.kt 89.47% <0%> (ø) 5% <0%> (ø) ⬇️
...rbosch/detekt/rules/AllowedExceptionNamePattern.kt

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 5874840...1e4d1ba. Read the comment docs.

@arturbosch
Copy link
Member

I also appreciate your work and investigations on this one!
With this blocker out, the way is open for 1.0 and starting 'normal' versioning.
We can deliver this feature better in a minor 1.X update later :)

@arturbosch arturbosch added this to the 1.0.0 milestone Jul 28, 2019
This pull request was closed.
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.

4 participants