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

Feature/restrict is properties #2779

Merged
merged 9 commits into from
Jun 20, 2020

Conversation

imanushin
Copy link
Contributor

Fix for #2701

What I covered:

  1. Property/arguments with val/var shouldn't start with "is" for non-boolean types
  2. isengard property name is allowed for any type, isDefault is disallowed
  3. Generic types are out of scope (to be formal, it is better to prohibit them too, however this question isn't so simple for me, need help)

This rule is enabled by default in the naming ruleset, it doesn't have any parameters.

@imanushin imanushin force-pushed the feature/restrict-is-properties branch from e7497ca to dea4f4e Compare June 7, 2020 18:10
@codecov
Copy link

codecov bot commented Jun 7, 2020

Codecov Report

Merging #2779 into master will increase coverage by 0.02%.
The diff coverage is 86.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2779      +/-   ##
============================================
+ Coverage     80.52%   80.54%   +0.02%     
- Complexity     2323     2335      +12     
============================================
  Files           386      387       +1     
  Lines          6957     6987      +30     
  Branches       1262     1265       +3     
============================================
+ Hits           5602     5628      +26     
  Misses          726      726              
- Partials        629      633       +4     
Impacted Files Coverage Δ Complexity Δ
...arturbosch/detekt/rules/naming/IsPropertyNaming.kt 85.18% <85.18%> (ø) 12.00 <12.00> (?)
...tlab/arturbosch/detekt/rules/naming/NamingRules.kt 93.33% <100.00%> (+0.35%) 18.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 2e07d5e...f7c3987. Read the comment docs.

@cortinico
Copy link
Member

Thanks for your contribution @imanushin 🎉

The CI is red because there are a couple of detekt failures on your code (NoWildcardImports, etc.) plus the documentation needs to be re-generated.

If you run ./gradlew build on your branch you'll get the same failures and the docs will be re-generated. Once your build is green locally, the CI should also be green.

@BraisGabin
Copy link
Member

Usually we don't activate a new rule by default. It's safer to have it disable by default and get feedback from the users that enabled it. If there are not issues we can activate it later.

@arturbosch
Copy link
Member

Please make sure to rebase on top of #2782 when the PR is merged 💪

@imanushin imanushin force-pushed the feature/restrict-is-properties branch 2 times, most recently from 505f50d to 9e9127e Compare June 14, 2020 18:04
@imanushin imanushin force-pushed the feature/restrict-is-properties branch from f3a84ac to cff284f Compare June 14, 2020 19:09
@imanushin imanushin force-pushed the feature/restrict-is-properties branch 3 times, most recently from 6d77faf to 8123c7c Compare June 15, 2020 19:42
@imanushin
Copy link
Contributor Author

Finally I got green build, however the following test was removed. Could you please help, do you know why it isn't work on ubuntu linux (it perfectly works at my Windows 10 computer)?

Removed test:

it("should not detect Java Boolean") {
    val code = """
    class O {
        var isDefault: java.lang.Boolean = java.lang.Boolean.FALSE // replacing to "false" doesn't work too
    }
"""
    val findings = subject.compileAndLintWithContext(env, code)
    
    assertThat(findings).isEmpty()
}

@cortinico
Copy link
Member

cortinico commented Jun 15, 2020

Could you please help, do you know why it isn't work on ubuntu linux (it perfectly works at my Windows 10 computer)?

I guess the failure was due to the fact that on the build instance ubuntu-latest+java8 we also compile the test snippets (to make sure you're actually writing valid Kotlin code inside the snippets). We do it only there as it will slow down the CI too much.

Given that you rebased on top of master, now you see that there is a job called compile-test-snippets that will do exactly that.

To test the same scenario on your machine you can invoke:

./gradlew test -x ":detekt-gradle-plugin:test" -Pcompile-test-snippets=true

That will compile the snippets and tell you what's wrong with your code.

As an alternative, you can use play.kotlinlang.org: https://pl.kotl.in/W9SFqrlPJ
Just copy your snippet there and it should be all green. In your specific case, you can replace with:

class O {
    var isDefault: java.lang.Boolean = java.lang.Boolean(false)
}

EDIT: Typos

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

A couple of minor typos and we're good to go 👍

@arturbosch arturbosch added this to the 1.10.0 milestone Jun 16, 2020
@imanushin
Copy link
Contributor Author

@arturbosch , much appreciated the text fix.

I'll return tests today evening (I couldn't push from working computer, sorry). And I'm ok with merging now, I can add the tests in the separate PR.

@imanushin imanushin force-pushed the feature/restrict-is-properties branch from b352c6f to 9b9bcca Compare June 16, 2020 20:44
@imanushin
Copy link
Contributor Author

Documentation was regenerated, tests were reverted.

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Awesome 👍

@arturbosch arturbosch merged commit 194ace6 into detekt:master Jun 20, 2020
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.

None yet

4 participants