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

Replace @requiresTypeResolution kdoc tag with @RequiresTypeResolution annotation #3579

Merged
merged 11 commits into from
Mar 27, 2021

Conversation

marschwar
Copy link
Contributor

This PR belongs to #3562 and replaces the @requiresTypeResolution kdoc tag with a @RequiresTypeResolution annotation.

There are a few things that I would like to point out/discuss:

  • I still have some formatting issues. That is why there is one commit that just reformats the code according to the ktlint plugin
  • I added the annotation to the detekt API which might be a bit premature. Maybe it should be placed in internal until it is stable?
  • I started out adding @UnstableApi to the annotation but that would all rules to require to add @OptIn(UnstableApi::class) which does not feel right either.
  • Adding a @Suppress("TooManyFunctions") to the RuleVisitor does not feel great, but I thinkt the code reads much better with those extension functions. Suggestions are welcome.


@Suppress("TooManyFunctions")
Copy link
Member

Choose a reason for hiding this comment

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

Let's move the methods of KtClassOrObject.requiresTypeResolution() and KtClassOrObject.isAnnotatedWith() to detekt-api. I think they should be exposed and can be reused.

Hopefully we won't require suppressing TooManyFunctions

@chao2zhang chao2zhang added this to the 1.17.0 milestone Mar 19, 2021
@codecov
Copy link

codecov bot commented Mar 19, 2021

Codecov Report

Merging #3579 (248b738) into main (619aa19) will increase coverage by 0.06%.
The diff coverage is 83.87%.

❗ Current head 248b738 differs from pull request most recent head acc5f7b. Consider uploading reports for the commit acc5f7b to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #3579      +/-   ##
============================================
+ Coverage     77.53%   77.59%   +0.06%     
+ Complexity     2837     2832       -5     
============================================
  Files           464      465       +1     
  Lines          8781     8805      +24     
  Branches       1720     1719       -1     
============================================
+ Hits           6808     6832      +24     
  Misses         1047     1047              
  Partials        926      926              
Impacted Files Coverage Δ Complexity Δ
...urbosch/detekt/generator/collection/Annotations.kt 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...turbosch/detekt/rules/complexity/NamedArguments.kt 83.33% <ø> (ø) 15.00 <0.00> (ø)
...etekt/rules/coroutines/RedundantSuspendModifier.kt 57.14% <ø> (ø) 20.00 <0.00> (ø)
...sch/detekt/rules/coroutines/SleepInsteadOfDelay.kt 73.91% <ø> (ø) 10.00 <0.00> (ø)
...t/rules/coroutines/SuspendFunWithFlowReturnType.kt 71.42% <ø> (ø) 7.00 <0.00> (ø)
...gitlab/arturbosch/detekt/rules/bugs/Deprecation.kt 80.00% <ø> (ø) 6.00 <0.00> (ø)
...h/detekt/rules/bugs/DontDowncastCollectionTypes.kt 81.25% <ø> (ø) 8.00 <0.00> (ø)
...ab/arturbosch/detekt/rules/bugs/ExitOutsideMain.kt 66.66% <ø> (ø) 4.00 <0.00> (ø)
...ab/arturbosch/detekt/rules/bugs/HasPlatformType.kt 68.00% <ø> (ø) 11.00 <0.00> (ø)
...rbosch/detekt/rules/bugs/ImplicitUnitReturnType.kt 81.48% <ø> (ø) 10.00 <0.00> (ø)
... and 36 more

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 619aa19...acc5f7b. Read the comment docs.

@marschwar
Copy link
Contributor Author

I removed the annotation from the exposed api by moving it to the internal package. Also the retention is changed to SOURCE so it is not available at runtime. This way we should be fairly flexible when it comes to changing the annotation(s) without breaking peoples code.

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.

I still have some formatting issues. That is why there is one commit that just reformats the code according to the ktlint plugin

I wonder why this is happening 🤔

I started out adding @UnstableApi to the annotation but that would all rules to require to add @OptIn(UnstableApi::class) which does not feel right either.

I think @UnstableApi is a bit of an overkill. Having the annotation inside .internal is sufficient. Also I don't know how many users will start relying on this annotation right after, so I won't worry too much about protecting this API.

Copy link
Member

@chao2zhang chao2zhang left a comment

Choose a reason for hiding this comment

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

@cortinico I was wondering if this should be in external API. The problem is that we should also allow custom rules to be annotated correctly, #3577 is a good example to fully skip type resolution rules.

Regardless, this PR looks good as it stands for now. I don't mind if we "escalate" these apis later.

Base automatically changed from master to main March 21, 2021 18:44
@marschwar marschwar force-pushed the annotations-requires-type-resolution branch 2 times, most recently from 9bd9427 to b520fef Compare March 22, 2021 17:19
@BraisGabin BraisGabin mentioned this pull request Mar 22, 2021
@cortinico
Copy link
Member

@cortinico I was wondering if this should be in external API. The problem is that we should also allow custom rules to be annotated correctly

I believe yes, this should be an external API. Something to remember for the future 👍

@marschwar marschwar force-pushed the annotations-requires-type-resolution branch from b520fef to acc5f7b Compare March 25, 2021 15:44
@chao2zhang chao2zhang merged commit 7c4a690 into detekt:main Mar 27, 2021
@marschwar marschwar deleted the annotations-requires-type-resolution branch March 27, 2021 20:52
@cortinico cortinico added the notable changes Marker for notable changes in the changelog label May 1, 2021
TWiStErRob added a commit to atulgpt/detekt that referenced this pull request May 19, 2023
> Task :detekt-test:detektMain
Property 'style>ForbiddenComment>values' is deprecated. Use `comments` instead, make sure you escape your text for Regular Expressions..

History:
@requiresTypeResolution: detekt#3579 + 13a8389
@author: detekt#1776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notable changes Marker for notable changes in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants