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 warning to suggest to use inline classes when a class has single property #716

Merged
merged 21 commits into from
Jan 27, 2021

Conversation

aktsay6
Copy link
Collaborator

@aktsay6 aktsay6 commented Jan 19, 2021

What's done:

  • Logic made
  • Added warn tests

This pull request closes #698

Actions checklist

  • Implemented Rule, added Warnings
  • Added tests on checks
  • Added tests on fixers
  • Updated diktat-analysis.yml
  • Updated available-rules.md

Fixme

Since it is still experimental feature it shouldn't be fixed

### What's done:
  * Logic made
  * Added warn tests
@aktsay6 aktsay6 added the enhancement New feature or request label Jan 19, 2021
@codecov
Copy link

codecov bot commented Jan 19, 2021

Codecov Report

Merging #716 (824af31) into master (00a00ae) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #716      +/-   ##
============================================
- Coverage     80.15%   80.12%   -0.03%     
- Complexity     1922     1944      +22     
============================================
  Files            91       92       +1     
  Lines          4958     4982      +24     
  Branches       1584     1595      +11     
============================================
+ Hits           3974     3992      +18     
  Misses          241      241              
- Partials        743      749       +6     
Flag Coverage Δ Complexity Δ
unittests 80.12% <75.00%> (-0.03%) 0.00 <22.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...at-rules/src/main/kotlin/generated/WarningNames.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)
...uleset/rules/chapter6/classes/InlineClassesRule.kt 72.72% <72.72%> (ø) 22.00 <22.00> (?)
...tlin/org/cqfn/diktat/ruleset/constants/Warnings.kt 97.76% <100.00%> (+0.01%) 11.00 <0.00> (ø)
...cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt 93.39% <100.00%> (+0.06%) 11.00 <0.00> (ø)

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 00a00ae...824af31. Read the comment docs.

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
diktat-analysis.yml Outdated Show resolved Hide resolved
info/guide/guide-chapter-6.md Outdated Show resolved Hide resolved

@Test
@Tag(WarningNames.INLINE_CLASS_CAN_BE_USED)
fun `should not trigger on class with internal constructor`() {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure it's prohibited? IDEA doesn't seem to highlight it in red.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As IDEA highlighted, constructor should be public, otherwise class can't be inline

Copy link
Member

Choose a reason for hiding this comment

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

idk, my idea doesn't :)
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Strange, I have this
image
IntelliJ IDEA 2020.3 (Ultimate Edition)
Build #IU-203.5981.155, built on December 1, 2020

Copy link
Member

Choose a reason for hiding this comment

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

hmm, do you try this code in diktat? I would expect results to be the same with same kotlin version. Which version of kotlin plugin do you have in idea? My is 203-1.4.30-RC-IJ6682.9 for idea 2020.3.2. Let's check if this code compiles with kotlin 1.4.21.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now I have 1.4.21 and this code doesn't compile

Copy link
Member

Choose a reason for hiding this comment

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

OK, that is a bug in IDEA plugin. Maybe, it'll be allowed in 1.4.30, but let's allow only public for now.

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
} else if (classPsi.getProperties().isEmpty() && classPsi.hasExplicitPrimaryConstructor()) {
return classPsi.primaryConstructorParameters.size == 1 &&
!classPsi.primaryConstructorParameters.first().node.hasChildOfType(VAR_KEYWORD) &&
classPsi.primaryConstructorModifierList == null
classPsi.primaryConstructor?.visibilityModifierType()?.value ?: "public" == "public"
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
classPsi.primaryConstructor?.visibilityModifierType()?.value ?: "public" == "public"
classPsi.primaryConstructor?.visibilityModifierType()?.value?.let { it == "public" } ?: true

Maybe, something like this?

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
# Conflicts:
#	diktat-rules/src/main/kotlin/generated/WarningNames.kt
Copy link
Member

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

go go go

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
Copy link
Collaborator

@kentr0w kentr0w left a comment

Choose a reason for hiding this comment

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

lgfm

@@ -357,6 +357,9 @@ fun main() {
}
```

### <a name="r6.1.12"></a> 6.1.12 Prefer Inline classes when a class has a single property
Copy link
Member

@orchestr7 orchestr7 Jan 26, 2021

Choose a reason for hiding this comment

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

example?
Bad example - Good example

and more info about inline classes

@@ -17,6 +17,7 @@ import kotlinx.serialization.encodeToString
* Special test that checks that developer has not forgotten to add his warning to a diktat-analysis.yml
* This file is needed to be in tact with latest changes in Warnings.kt
*/
@Suppress("INLINE_CLASS_CAN_BE_USED")
Copy link
Member

Choose a reason for hiding this comment

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

may be inline this Test?

Copy link
Member

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

please look at comments

### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
# Conflicts:
#	diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/DiktatRuleSetProvider.kt
### What's done:
  * Fixed bugs
### What's done:
  * Fixed bugs
Copy link
Member

@orchestr7 orchestr7 left a comment

Choose a reason for hiding this comment

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

lgtm

@aktsay6 aktsay6 merged commit 05c3ebb into master Jan 27, 2021
@aktsay6 aktsay6 deleted the feature/inline-classes(#698) branch January 27, 2021 09:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warning to suggest to use inline classes when a class has single property
4 participants