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

Create Rule Set For Libraries #5360

Merged
merged 7 commits into from Oct 15, 2022
Merged

Conversation

VitalyVPinchuk
Copy link
Contributor

@VitalyVPinchuk VitalyVPinchuk commented Sep 28, 2022

Closes #4940
Create ruleset plugin for libraries.
Move the following rules to it:

ForbiddenPublicDataClass
LibraryCodeMustSpecifyReturnType
LibraryEntitiesShouldNotBePublic

@github-actions
Copy link

github-actions bot commented Sep 28, 2022

Warnings
⚠️ This PR is approved with no milestone set. If merged, it won't appear in the Detekt release notes.
Messages
📖 Thanks for adding a new rule to Detekt ❤️

Generated by 🚫 dangerJS against c0bf789

@cortinico
Copy link
Member

Create ruleset plugin for libraries.

Was this discussed already in the past? I'm fine with this change but maybe we should focus on having a 'library mode' which is effectively a set of config options aimed at libraries?

@3flex
Copy link
Member

3flex commented Sep 30, 2022

Agree that's a better approach. The rules/"library mode" can then be dynamically enabled based on whether Kotlin's explicit API mode is enabled or not.

@VitalyVPinchuk
Copy link
Contributor Author

Hello!
As far as I understood from a talk to @BraisGabin a new module was needed so I did it exactly like "rule authors".
If that's not the case I'm ready to rework it, just need a little bit of guidance.

@BraisGabin
Copy link
Member

We talk about this at #4940. Explicit api is not directly related with library development. It's true that it was designed for that use case but I use it on my daily basis. I consider that the major issue of kotlin is to make public the default visibility. For that reason, the explicit API helps me to "fix" it.

So, instead of trying to be "clever" about when to enable this rules it's better to move it to a different rule-set. I see two option here.

And why to move it outside the default rule set and moving it to a first-party plugin? Because we try to fix #4940. Right now those rules have a really strange workaround so allRules doesn't run them.

Copy link
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.

We should add the old rules as deprecated so our users get notified that those rules were moved. To be more precisse. deprecate their old configuration.

config/detekt/detekt.yml Show resolved Hide resolved
Copy link
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.

LGTM

Hardcoded strings of migrated rules are added to deprecation.properties.
Include :detekt-rules-libraries project in the root build.gradle.kts
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #5360 (c0bf789) into main (dbb6ade) will decrease coverage by 0.07%.
The diff coverage is 50.00%.

@@             Coverage Diff              @@
##               main    #5360      +/-   ##
============================================
- Coverage     86.09%   86.02%   -0.08%     
+ Complexity     3637     3609      -28     
============================================
  Files           516      513       -3     
  Lines         12098    12019      -79     
  Branches       2156     2147       -9     
============================================
- Hits          10416    10339      -77     
- Misses          613      616       +3     
+ Partials       1069     1064       -5     
Impacted Files Coverage Δ
...itlab/arturbosch/detekt/generator/DetektPrinter.kt 20.00% <0.00%> (-3.34%) ⬇️
...rturbosch/detekt/rules/style/StyleGuideProvider.kt 100.00% <ø> (ø)
...osch/detekt/generator/printer/DeprecatedPrinter.kt 100.00% <100.00%> (ø)
...etekt/generator/printer/defaultconfig/Exclusion.kt 96.29% <100.00%> (-0.58%) ⬇️
...bosch/detekt/libraries/ForbiddenPublicDataClass.kt
...tekt/libraries/LibraryCodeMustSpecifyReturnType.kt
...tekt/libraries/LibraryEntitiesShouldNotBePublic.kt

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@VitalyVPinchuk
Copy link
Contributor Author

What could be wrong with :detekt-parser:test?
I use latest macos with Temurin-17.0.4.1+1 but can't reproduce it.

KtCompilerSpec > parallel construction of KtCompilers should be thread safe() FAILED
    java.lang.AssertionError: Breakpoint wasn't hit within 10000 milliseconds

if (!extensionArea.hasExtensionPoint(extension)) {
ConTesterBreakpoint.defineBreakpoint("DetektPomModel.registerExtensionPoint") {
extensionArea == getRootArea()

@BraisGabin
Copy link
Member

It seems more a flaky execution. I just launch it again.

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.

LGTM 👍 Let's include this inside RC2

@cortinico cortinico added this to the 1.22.0 milestone Oct 15, 2022
@cortinico cortinico merged commit b620bd5 into detekt:main Oct 15, 2022
@@ -0,0 +1,11 @@
libraries:
Copy link
Member

Choose a reason for hiding this comment

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

@BraisGabin is this an internal file to Detekt, or ruleset plugin authors can also provide this file?

Copy link
Member

Choose a reason for hiding this comment

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

This one targets library authors. I don't know if this one could be useful for plugin authors. I don't recall the name now but on this version we are going to add other rule set only for rule authors.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I meant exactly this file, the config yml. Because src/main/resources will get packaged into the jar by default, and can be opened using a Class. If this is only used for generating something internally, it might be better to move it out of this folder so it doesn't get delivered. So my question is if it's being used by some mechanism outside of docs generation.

Copy link
Member

Choose a reason for hiding this comment

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

Looking forward to rules for rules! That's a great plugin we really need.

Copy link
Member

Choose a reason for hiding this comment

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

If this is only used for generating something internally, it might be better to move it out of this folder so it doesn't get delivered. So my question is if it's being used by some mechanism outside of docs generation.

This file is used when generating the default config file for the users as well as during validation, so it's definitelly needed and should be shipped

Copy link
Member

Choose a reason for hiding this comment

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

Thank you :)

@dmitry-weirdo
Copy link

dmitry-weirdo commented Jan 4, 2023

What to do if setting these properties in libraries (according to the schema) causes this failure:

- Property 'libraries' is misspelled or does not exist.

This happens for me when using the maven plugin, not gradle — see Ozsie/detekt-maven-plugin#169.

Also, not like for formatting, there is no such maven artifact like detekt-formatting as mentioned in #5304 (reply in thread).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build notable changes Marker for notable changes in the changelog rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contradictory configuration of ForbiddenPublicDataClass
6 participants