Skip to content

Fix issue IDs for ReferentialEquality and DoubleMutability#4040

Merged
BraisGabin merged 1 commit into
detekt:mainfrom
geojakes:gj/FixId
Aug 16, 2021
Merged

Fix issue IDs for ReferentialEquality and DoubleMutability#4040
BraisGabin merged 1 commit into
detekt:mainfrom
geojakes:gj/FixId

Conversation

@geojakes
Copy link
Copy Markdown
Contributor

@geojakes geojakes commented Aug 13, 2021

Trying to fix for #4039
I think the issue ID created does not match the configuration and hence does not affect it the same way as expected.

Disabling the DoubleMutabilityForCollection rule as it seems the project has quite a few of said issues

Copy link
Copy Markdown
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 for the fix! I guess this strings were forgotten during the rule refactoring.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 13, 2021

Codecov Report

Merging #4040 (c472dbe) into main (31106ff) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##               main    #4040   +/-   ##
=========================================
  Coverage     83.56%   83.57%           
- Complexity     3182     3186    +4     
=========================================
  Files           459      459           
  Lines          9092     9097    +5     
  Branches       1771     1772    +1     
=========================================
+ Hits           7598     7603    +5     
  Misses          561      561           
  Partials        933      933           
Impacted Files Coverage Δ
.../kotlin/io/gitlab/arturbosch/detekt/api/Context.kt 70.00% <100.00%> (ø)
...osch/detekt/rules/bugs/AvoidReferentialEquality.kt 91.30% <100.00%> (ø)
...detekt/rules/bugs/DoubleMutabilityForCollection.kt 88.46% <100.00%> (ø)
...turbosch/detekt/formatting/wrappers/Indentation.kt 100.00% <0.00%> (ø)
...lab/arturbosch/detekt/formatting/FormattingRule.kt 97.67% <0.00%> (+0.05%) ⬆️

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 31106ff...c472dbe. Read the comment docs.

@geojakes
Copy link
Copy Markdown
Contributor Author

@schalkms Np! :) How do I get this merged?

@schalkms
Copy link
Copy Markdown
Member

@geojakes I think the following issue needs to be fixed. Then this PR is ready to be merged.

DoubleMutabilityForCollection - [_findings] at /home/runner/work/detekt/detekt/detekt-api/src/main/kotlin/io/gitlab/arturbosch/detekt/api/Context.kt:53:5

@geojakes
Copy link
Copy Markdown
Contributor Author

@schalkms Looks like that's unrelated to the PR since that area isn't being touched at all. I ran a detektMain and detektTest on the repo, seems like there are many more locations with the same issue

@geojakes
Copy link
Copy Markdown
Contributor Author

@schalkms Made the relevant changes to that specific file though

@schalkms
Copy link
Copy Markdown
Member

Looks like that's unrelated to the PR since that area isn't being touched at all. I ran a detektMain and detektTest on the repo, seems like there are many more locations with the same issue

Yes, this is kind of unrelated to this PR. However, the fixed rule now yields errors for the detekt codebase, since detekt's static analysis capability is applied to itself. This approach is also called dogfooding.

@geojakes
Copy link
Copy Markdown
Contributor Author

geojakes commented Aug 13, 2021

@schalkms Looks like once that is fixed, it yielded more errors

potential-bugs - 15min debt
	DoubleMutabilityForCollection - [classesMap] at /home/runner/work/detekt/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt:42:5
	DoubleMutabilityForCollection - [ruleNames] at /home/runner/work/detekt/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt:105:5
	DoubleMutabilityForCollection - [ruleProperties] at /home/runner/work/detekt/detekt/detekt-generator/src/main/kotlin/io/gitlab/arturbosch/detekt/generator/collection/MultiRuleCollector.kt:107:5

These look a little more complicated to fix. Maybe we should turn this rule off and open a new issue to address the same?

@geojakes
Copy link
Copy Markdown
Contributor Author

geojakes commented Aug 14, 2021

Updated to disable the rule, adding a new issue to fix the same: #4042

@geojakes
Copy link
Copy Markdown
Contributor Author

Looks like this scan is broken in the main branch as well...

Copy link
Copy Markdown
Member

@BraisGabin BraisGabin left a comment

Choose a reason for hiding this comment

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

Good catch! But... This seems like an issue in the config generation... We should use the issue name, no the class name.

@geojakes
Copy link
Copy Markdown
Contributor Author

@BraisGabin I somehow get weird outputs from the repo. When I apply my fix, it picks up the DoubleMutability issues, but does allow me to deactivate it from the config. On the other hand, another project that I work on, runs 1.18.0, picks up the DoubleMutability issue, but does not allow deactivation via config. Is this because of how detekt does its dogfooding?

@BraisGabin
Copy link
Copy Markdown
Member

No, no, your fix is perfect. But I think that we could fix the generator to prevent this kind of issues. But that's another topic.

@geojakes
Copy link
Copy Markdown
Contributor Author

@schalkms @BraisGabin Does this look okay to merge?

@BraisGabin BraisGabin merged commit 023fbc6 into detekt:main Aug 16, 2021
@BraisGabin
Copy link
Copy Markdown
Member

It does.

@geojakes
Copy link
Copy Markdown
Contributor Author

@BraisGabin What is the process for a micro version release?

@BraisGabin
Copy link
Copy Markdown
Member

There is not a defined process or ETA. But this kind of fix versions are released "fast"

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants