[#272, #181, #174] Upgrade to latest Detekt and address issues accord… #1187

mikezx6r · 2018-12-10T03:48:58Z

…ingly

This includes replacing direct execution with Gradle plugin, migrating the configuration to new format, and either fixing, or suppressing issues in the existing code base.

Note that the new version of Detekt no longer manages/verifies formatting options. They feel other tools are better suited to that (IntelliJ formatting etc).

I attempted to maintain the same rules in the project, and generally suppress issues that exist in the code.

Baseline is not currently enabled as I personally prefer @Suppress instead, but that can be changed easily enough.

pakoito · 2018-12-10T10:50:14Z

Seems to me that the baseline is behind master or something?

raulraja · 2018-12-19T18:42:43Z

Needs merge from master

mikezx6r · 2018-12-20T13:11:08Z

@raulraja My apologies. This fell by the wayside when I ran into build issues due to the process taking too long, and therefore causing the builds to fail.

While browsing the Detekt issues, found this comment from the creator:
detekt/detekt#1318 (comment)

Using ci/bitrise, it appears to execute, so I can continue with the upgrade, and migration to the Gradle plugin.

If you'd prefer to keep the build a bit faster, I can upgrade Detekt, but continue to use a custom task to execute it.

Let me know which you prefer, or if you want some firmer numbers, and I'll proceed accordingly.

raulraja · 2018-12-20T14:13:51Z

I think the approach with custom task that runs before build and the upgrade would be best going forward

mikezx6r · 2018-12-26T11:42:46Z

Back on this, and focused on getting it done. Re-read the comment about Detekt and large projects again, as well as reviewing the existing definition for Detekt.

The comment suggests running Detekt once for the entire project, whereas the way it was defined in Arrow would run it on each project/module. So I doubt there's a significant difference between creating a custom task that is executed by each project vs having the Gradle plugin add a task to each project.

The Detekt Gradle plugin simplifies configuration, and allows for execution on Windows, Linux etc w/o any custom work. I'll do a bit of testing to compare, but I'm beginning to suspect the difference is more checks in Detekt as opposed to any Gradle overhead.

I'll keep you posted as I proceed, but wanted to let you know I'm still on this.

mikezx6r · 2018-12-30T14:14:37Z

@raulraja If you have a chance, it now builds, and works on Windows.

I was originally making some code changes, but found some areas of the code that although technically, correct, weren't correct at runtime. Didn't dig too deep, but rather just added appropriate Suppress annotations.

Let me know if there's anything else required before this can be merged

raulraja

@mikezx6r thanks so much for all this great work. I added a request to discard some of the directives all together in the detekt config in order to reduce the presence of annotations all over the place.
Things like UnusedPrivateMember, ComplexInterface, MethodOverloading etc can just go away.
Once that is addressed this is ready to merge.

raulraja · 2018-12-30T18:47:24Z

modules/ank/arrow-ank/src/main/kotlin/arrow/ank/formatter.kt

@@ -1,3 +1,4 @@
+@file:Suppress("UnusedPrivateMember")


Can we add the ones that repeat frequently to the settings in detekt to ignore or simply create a new baseline for what we have?. Having the updated detekt is great but having all these annotations all over makes me think that if these are so common in Arrow perhaps they should be part of our baseline or be ignored in the detekt config.

Sure. I just made the build pass as I didn't want to make judgements like this.

raulraja · 2018-12-30T18:48:18Z

...core/arrow-annotations-processor/src/main/java/arrow/higherkinds/HigherKindsFileGenerator.kt

@@ -7,9 +7,13 @@ import me.eugeniomarletti.kotlin.metadata.shadow.metadata.ProtoBuf
 import java.io.File
 import javax.lang.model.element.Name

+@Suppress("MayBeConst")


If these can be constants then they may as well be declared as such or remove this check from our detekt config.

I had a number of issues when I made seemingly trivial changes, but let me try again and see if it works ok.

raulraja · 2018-12-30T18:52:31Z

modules/core/arrow-instances-core/src/main/kotlin/arrow/instances/tuple.kt

@@ -498,6 +501,7 @@ interface Tuple9EqInstance<A, B, C, D, E, F, G, H, I> : Eq<Tuple9<A, B, C, D, E,
 }

 @extension
+@Suppress("ComplexInterface")


We definitely don't want the ComplexInterface directive running as we have many cases where arity of methods forces us to the kind of stuff you see in TupleN. This is a lang limitation not Arrow's and Detekt should not have an opinion about it.

raulraja · 2018-12-30T18:52:53Z

modules/core/arrow-syntax/src/test/kotlin/arrow/syntax/test/function.kt

@@ -109,8 +109,8 @@ class FunctionSyntaxTest : UnitSpec() {
      val b = { counterB++ }.memoize()


-      (1..5).forEach { a() }
-      (1..5).forEach { b() }
+      repeat(5) { a() }


raulraja · 2018-12-30T18:54:06Z

...row-effects-kotlinx-coroutines-instances/src/main/kotlin/arrow/effects/DeferredKInstances.kt

@@ -1,3 +1,4 @@
+@file:Suppress("UnusedImports")


If there are unused imports can we address them or is this a false positive?

This is a detekt issue. If an import uses an alias, Detekt doesn't recognize that it's used. I'll ensure there's a ticket for it on the Detekt issues list.

raulraja · 2018-12-30T18:55:17Z

modules/effects/arrow-effects-reactor/src/main/kotlin/arrow/effects/MonoKConnection.kt

@@ -18,7 +18,7 @@ typealias MonoKProcF<A> = (MonoKConnection, (Either<Throwable, A>) -> Unit) -> M
 *
 * @see MonoK.async
 */
-@Suppress("FunctionName")


We don't care about FunctionName either because we have several factories that are upper-cased to match the type constructor invocation style in other places

raulraja · 2018-12-30T18:56:50Z

modules/effects/arrow-effects/src/main/kotlin/arrow/effects/IORunLoop.kt

@@ -13,7 +13,7 @@ private typealias BindF = (Any?) -> IO<Any?>
 private typealias CallStack = ArrayStack<BindF>
 private typealias Callback = (Either<Throwable, Any?>) -> Unit

-@Suppress("UNCHECKED_CAST")
+@Suppress("UNCHECKED_CAST", "ReturnCount", "ComplexMethod")


These in contrast are fine since this really is an odd method of the IO runtime in Arrow that is described exactly like this for performance reasons but normal coding practices should not be as aggressive in terms of optimizing for performance.

…nd address issues accordingly This includes replacing direct execution with Gradle plugin, migrating the configuration to new format, and either fixing, or suppressing issues in the existing code base.

mikezx6r · 2018-12-31T14:33:00Z

@raul. I believe I incorporated all your suggestions, so when you have a chance...

raulraja

awesome work and thanks so much!

mikezx6r · 2018-12-31T14:43:42Z

Took longer than I would have liked, but that happens... But it feels good to have done it.

You may want to follow up on the linked issues to see if this truly does resolve them or not.

mikezx6r force-pushed the mw/detekt-upgrade branch from 1bda03d to 83779fb Compare December 10, 2018 04:09

mikezx6r force-pushed the mw/detekt-upgrade branch from 83779fb to dbf48c5 Compare December 10, 2018 11:02

mikezx6r force-pushed the mw/detekt-upgrade branch 4 times, most recently from 382dc7c to e9574be Compare December 30, 2018 13:01

raulraja requested changes Dec 30, 2018

View reviewed changes

mikezx6r added 2 commits December 30, 2018 20:32

Aid diagnosis of test failures by showing full exception details

b12b5f2

Make test Windows compatible

c96120c

mikezx6r force-pushed the mw/detekt-upgrade branch from e9574be to bc92fb6 Compare December 31, 2018 02:41

[arrow-kt#272, arrow-kt#181, arrow-kt#174] Upgrade to latest Detekt a…

aeb703a

…nd address issues accordingly This includes replacing direct execution with Gradle plugin, migrating the configuration to new format, and either fixing, or suppressing issues in the existing code base.

mikezx6r force-pushed the mw/detekt-upgrade branch from bc92fb6 to aeb703a Compare December 31, 2018 03:55

raulraja approved these changes Dec 31, 2018

View reviewed changes

raulraja merged commit 97fcf18 into arrow-kt:master Dec 31, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#272, #181, #174] Upgrade to latest Detekt and address issues accord… #1187

[#272, #181, #174] Upgrade to latest Detekt and address issues accord… #1187

mikezx6r commented Dec 10, 2018

pakoito commented Dec 10, 2018

raulraja commented Dec 19, 2018

mikezx6r commented Dec 20, 2018

raulraja commented Dec 20, 2018

mikezx6r commented Dec 26, 2018

mikezx6r commented Dec 30, 2018

raulraja left a comment

raulraja Dec 30, 2018 •

edited

Loading

mikezx6r Dec 31, 2018

raulraja Dec 30, 2018

mikezx6r Dec 31, 2018

raulraja Dec 30, 2018

raulraja Dec 30, 2018

raulraja Dec 30, 2018

mikezx6r Dec 31, 2018

raulraja Dec 30, 2018

raulraja Dec 30, 2018

mikezx6r commented Dec 31, 2018

raulraja left a comment

mikezx6r commented Dec 31, 2018

[#272, #181, #174] Upgrade to latest Detekt and address issues accord… #1187

[#272, #181, #174] Upgrade to latest Detekt and address issues accord… #1187

Conversation

mikezx6r commented Dec 10, 2018

pakoito commented Dec 10, 2018

raulraja commented Dec 19, 2018

mikezx6r commented Dec 20, 2018

raulraja commented Dec 20, 2018

mikezx6r commented Dec 26, 2018

mikezx6r commented Dec 30, 2018

raulraja left a comment

Choose a reason for hiding this comment

raulraja Dec 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikezx6r commented Dec 31, 2018

raulraja left a comment

Choose a reason for hiding this comment

mikezx6r commented Dec 31, 2018

raulraja Dec 30, 2018 •

edited

Loading