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 RedundantExplicitType rule #1900

Merged
merged 1 commit into from
Sep 14, 2019
Merged

Add RedundantExplicitType rule #1900

merged 1 commit into from
Sep 14, 2019

Conversation

3flex
Copy link
Member

@3flex 3flex commented Sep 7, 2019

Closes #336

Since this is just a port of the IntelliJ rule this uses identical logic, meaning this only checks local properties - it doesn't check for types that don't need to be declared for any other element. I think this conservative posture makes sense and there shouldn't be any false positives.

A future config option could be to enable or disable checking on top-level properties, but that's not required for now.

Blocked by #1880

@3flex 3flex added the blocked label Sep 7, 2019
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.

Looks good! I would prefer to refactor the BindingContext retrieval into an own generic method.

if (bindingContext == BindingContext.EMPTY) return
if (!property.isLocal) return
val typeReference = property.typeReference ?: return
val type =
Copy link
Member

Choose a reason for hiding this comment

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

I see this pattern for getting the BindingContext quite often.
Shall we refactor this into a generic method?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a good idea. Can also provide a warning if context not available so user knows the rule (or parts of the rule) were not run even when the rule was enabled?

Can that be done in another PR though?

Copy link
Member

@schalkms schalkms Sep 9, 2019

Choose a reason for hiding this comment

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

Yes, that makes sense. This should be done in a separate PR.
Currently, our documentation states:

--jvm-target
   EXPERIMENTAL: Target version of the generated JVM bytecode that was
   generated during compilation and is now being used for type resolution

I think it’s not experimental anymore. Furthermore, we also need to update the samples.

@schalkms
Copy link
Member

schalkms commented Sep 8, 2019

I would also acknowledge the work (kotlinc) somewhere in a license.md to comply with the Apache 2.0 license.

@3flex
Copy link
Member Author

3flex commented Sep 13, 2019

I would also acknowledge the work (kotlinc) somewhere in a license.md to comply with the Apache 2.0 license.

I'm not clear on what actually needs to be done to provide proper attribution - I know detekt and Kotlin compiler have Apache 2.0 licenses which is good, but in terms of correct attribution, is the reference in the source file to the original enough?

@schalkms
Copy link
Member

@3flex yes, I think so. I'll create an issue for that.

@codecov-io
Copy link

codecov-io commented Sep 14, 2019

Codecov Report

Merging #1900 into master will decrease coverage by 0.08%.
The diff coverage is 65.62%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1900      +/-   ##
============================================
- Coverage      80.4%   80.32%   -0.09%     
- Complexity     1973     1988      +15     
============================================
  Files           326      327       +1     
  Lines          5517     5549      +32     
  Branches       1027     1041      +14     
============================================
+ Hits           4436     4457      +21     
- Misses          543      545       +2     
- Partials        538      547       +9
Impacted Files Coverage Δ Complexity Δ
...bosch/detekt/rules/providers/StyleGuideProvider.kt 100% <100%> (ø) 3 <0> (ø) ⬇️
...rbosch/detekt/rules/style/RedundantExplicitType.kt 64.51% <64.51%> (ø) 15 <15> (?)

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 8803173...b5746f8. Read the comment docs.

@3flex 3flex merged commit 399e7f6 into detekt:master Sep 14, 2019
@3flex 3flex deleted the 336-fix branch September 14, 2019 06:15
@arturbosch arturbosch added this to the 1.1.0 milestone Sep 14, 2019
sowmyav24 pushed a commit to sowmyav24/detekt that referenced this pull request Sep 17, 2019
sowmyav24 pushed a commit to sowmyav24/detekt that referenced this pull request Sep 17, 2019
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
smyachenkov pushed a commit to smyachenkov/detekt that referenced this pull request Dec 9, 2019
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.

Rule: OptionalTypeDeclaration
4 participants