Resolve detekt findings #54

mpgirro · 2021-02-24T12:03:48Z

This PR closes #52 by resolving the issues found by detekt. I did not add any custom rules so far, but we might want to do that in the future(?). We've had mostly MaxLineLength and ReturnCount issues.

Some resulting constructs are definitely worth debating, and I'm not super happy with quite a few of them. I did not @Suppress too much and only when refactoring would have been very inconvenient.

coveralls · 2021-02-24T12:06:50Z

Pull Request Test Coverage Report for Build #30

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

400 of 413 (96.85%) changed or added relevant lines in 68 files are covered.
17 unchanged lines in 11 files lost coverage.
Overall coverage decreased (-0.3%) to 97.747%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/main/kotlin/dev/stalla/builder/validating/episode/ValidatingEpisodeBuilder.kt	9	10	90.0%
src/main/kotlin/dev/stalla/builder/validating/podcast/ValidatingPodcastItunesBuilder.kt	7	8	87.5%
src/main/kotlin/dev/stalla/builder/validating/podcast/ValidatingPodcastPodcastindexLockedBuilder.kt	5	6	83.33%
src/main/kotlin/dev/stalla/dom/DomBuilderFactory.kt	7	8	87.5%
src/main/kotlin/dev/stalla/parser/NamespaceParser.kt	2	4	50.0%
src/main/kotlin/dev/stalla/dom/DomWritingExtensions.kt	2	9	22.22%

Files with Coverage Reduction	New Missed Lines	%
src/main/kotlin/dev/stalla/dom/DomParsingExtensions.kt	1	95.45%
src/main/kotlin/dev/stalla/model/itunes/ItunesCategory.kt	1	99.21%
src/main/kotlin/dev/stalla/parser/namespace/AtomParser.kt	1	96.3%
src/main/kotlin/dev/stalla/parser/namespace/ContentParser.kt	1	85.71%
src/main/kotlin/dev/stalla/parser/namespace/GoogleplayParser.kt	1	95.65%
src/main/kotlin/dev/stalla/parser/namespace/ItunesParser.kt	1	97.37%
src/main/kotlin/dev/stalla/parser/namespace/PodcastindexParser.kt	1	98.25%
src/main/kotlin/dev/stalla/parser/namespace/PodloveSimpleChapterParser.kt	1	90.91%
src/main/kotlin/dev/stalla/model/StyledDuration.kt	2	98.26%
src/main/kotlin/dev/stalla/parser/namespace/FyydParser.kt	2	71.43%

Totals
Change from base Build #27:	-0.3%
Covered Lines:	1996
Relevant Lines:	2042

💛 - Coveralls

mpgirro · 2021-02-24T12:05:38Z

build.gradle.kts

-        jcenter()
+        jcenter {
+            content {
+                includeGroup("org.jetbrains.kotlinx")


Required for HTML reports. We might also use kotlinx for introducing truly immutable lists in the data classes again, for safety in Java.

Is that lib released? Immutable collections I mean.

Hm, I've just read up on it here, without checking the release status. Seems it's still in pre-release.

mpgirro · 2021-02-24T12:06:45Z

src/main/kotlin/dev/stalla/builder/AtomBuilder.kt

+    public fun addAllAuthorBuilder(authorBuilders: List<PersonBuilder>): AtomBuilder =
+        apply { authorBuilders.forEach(::addAuthorBuilder) }


These are in accordance with the formatting of expression-bodies (even though especially scope functions are nice on the same line as the =)

mpgirro · 2021-02-24T12:09:58Z

src/main/kotlin/dev/stalla/builder/episode/EpisodeBuilder.kt

@@ -1,3 +1,5 @@
+@file:Suppress("TooManyFunctions")


I've suppressed the TooManyFunctions in builders since we need at least as many methods in them as there are properties in their data class models.

I think we can simply suppress TooManyFunctions and TooManyParameters entirely, they don't really make too much sense when dealing with large models and files with top-level functions

Do you have a favourite config file at hand that we can use? Might be quicker than setting one up from scratch.

I'll try to find something

mpgirro · 2021-02-24T12:11:45Z

src/main/kotlin/dev/stalla/builder/episode/EpisodeGoogleplayBuilder.kt

-    override fun applyFrom(prototype: EpisodeGoogleplay?): EpisodeGoogleplayBuilder = whenNotNull(prototype) { googleplay ->
-        author(googleplay.author)
-        description(googleplay.description)
-        explicit(googleplay.explicit?.type)
-        block(googleplay.block)
-        imageBuilder(HrefOnlyImage.builder().applyFrom(googleplay.image))
+    override fun applyFrom(prototype: EpisodeGoogleplay?): EpisodeGoogleplayBuilder {
+        return whenNotNull(prototype) { googleplay ->
+            author(googleplay.author)
+            description(googleplay.description)
+            explicit(googleplay.explicit?.type)
+            block(googleplay.block)
+            imageBuilder(HrefOnlyImage.builder().applyFrom(googleplay.image))
+        }


Alternative here could be:

override fun applyFrom(prototype: EpisodeGoogleplay?): EpisodeGoogleplayBuilder = whenNotNull(prototype) { googleplay -> author(googleplay.author) description(googleplay.description) explicit(googleplay.explicit?.type) block(googleplay.block) imageBuilder(HrefOnlyImage.builder().applyFrom(googleplay.image)) }

but that's not really a single line expression body anymore.

It's not a single line but it it a single expression. Personal taste would prefer the expression body syntax, but easy either way

I also prefer the single expression tbh. Just wanted to stick to the convention as closely as possible. But if you're also preferring it, I'll change it then.

mpgirro · 2021-02-24T12:15:54Z

src/main/kotlin/dev/stalla/builder/validating/ValidatingAtomBuilder.kt

-    override fun addAuthorBuilder(authorBuilder: PersonBuilder): AtomBuilder = apply {
-        authorBuilders.add(authorBuilder)
-    }
+    override fun addAuthorBuilder(authorBuilder: PersonBuilder): AtomBuilder =
+        apply { authorBuilders.add(authorBuilder) }


For consistency, I've refactored these as well to new line expression bodies, even though the lines have not been too long here. Preferred the previous style more though.

mpgirro · 2021-02-24T12:30:37Z

src/main/kotlin/dev/stalla/dom/DomParsingExtensions.kt

+@Suppress("ReturnCount")
 internal fun Node.toItunesCategory(namespace: FeedNamespace? = null): ItunesCategory? {
    val categoryValue = getAttributeValueByName("text")?.trim() ?: return null
    val category = ItunesCategory.of(categoryValue) ?: return null


The alternatives I came up with were harder to read and still had too many returns 😅

Suppression is ok here too yep

mpgirro · 2021-02-24T12:31:49Z

src/main/kotlin/dev/stalla/parser/DateParser.kt

+    @Suppress("MagicNumber")
    private val dayOfWeek = mapOf(
        1L to "Mon",
        2L to "Tue",


Felt stupid to introduce constants here.

Absolutely :)

mpgirro · 2021-02-24T12:32:44Z

src/main/kotlin/dev/stalla/dom/DomParsingExtensions.kt

@@ -1,3 +1,5 @@
+@file:Suppress("TooManyFunctions")


Also suppressed TooManyFunctions in the extensions, since we have as many extensions as we like.

mpgirro · 2021-02-24T12:34:45Z

src/main/kotlin/dev/stalla/parser/namespace/AtomParser.kt

+            "contributor" -> ifCanBeParsed { toPersonBuilder(personBuilderProvider.createPersonBuilder(), namespace) }
+                ?.let(atomBuilder::addContributorBuilder)


Parsers suffered from both MaxLineLength and ReturnCount. Solved with ?.let and by only passing the function pointer.

mpgirro · 2021-02-24T12:36:46Z

src/main/kotlin/dev/stalla/parser/namespace/PodcastindexParser.kt

@@ -87,7 +83,10 @@ internal object PodcastindexParser : NamespaceParser() {
            .type(type)
    }

-    private fun Node.toSoundbiteBuilder(soundbiteBuilder: EpisodePodcastindexSoundbiteBuilder): EpisodePodcastindexSoundbiteBuilder? {
+    @Suppress("ReturnCount")


Again, not better idea for this method.

It's ok, it was already the smallest/simplest implementation in my mind 👯

rock3r

I'd turn off those rules mentioned in the comments, so we can spare ourselves some suppressions. Looks good for the rest!

…al config

mpgirro · 2021-02-26T18:05:04Z

I've disabled the mentioned rules. Since the ReturnCount is obsolete now, I've also reverted the parsers to their previous state, for better readability.

rock3r · 2021-02-26T18:18:40Z

Ace, I'll try to get you a detekt config file asap, may need to patch it up from other projects I have :)

rock3r · 2021-02-27T11:44:32Z

@mpgirro please see #57 where I add my own config and fix a bunch more issues. I am also testing SARIF support (more info in the PR)

Seb's detekt ruleset, and relevant fixes

rock3r · 2021-02-27T14:38:59Z

I think we can ship now 💯

mpgirro · 2021-02-27T14:41:19Z

Bit of stuff is still in my pipeline. Found another issue with specs actually 😅

rock3r · 2021-02-27T14:43:48Z

Huh, curious to see what. But can always be a separate PR :)

mpgirro added 12 commits February 20, 2021 12:21

Adds detekt

57d8049

Getting rid of most MaxLineLength findings of detekt

6e5ff28

More MaxLineLength fixes

71bcdf1

Add @Supress annotations

bda4a45

Fixing most ReturnCount issues

3264210

Use uniform style

28598c7

Apply function pointer directly in builders

d881922

Fix ImplicitDefaultLocale

58710e2

Fix remaining detekt issues

25a3aa5

Adjust style for consistency

31b1233

Apply more consistent styling based in coding conventions

7079194

Remove obsolete Android task

5feb60e

mpgirro added this to the 1.0.0 milestone Feb 24, 2021

mpgirro requested a review from rock3r February 24, 2021 12:03

mpgirro commented Feb 24, 2021

View reviewed changes

rock3r requested changes Feb 25, 2021

View reviewed changes

mpgirro added 3 commits February 25, 2021 21:29

Consistently use expression body syntax

4a59387

Disable detekt TooManyFunctions/ReturnCount/LongParameterList in glob…

9dc2e5e

…al config

Revert to old parser style, for better readability

df4663e

rock3r added 2 commits February 27, 2021 12:03

Import Seb's Detekt config, address warnings

33873a2

Generate SARIF report and upload it to GH actions

dac496d

rock3r mentioned this pull request Feb 27, 2021

Seb's detekt ruleset, and relevant fixes #57

Merged

Run GitHub Actions buildbot on all PRs

e466b5e

Merge pull request #57 from mpgirro/52_detekt_ruleset_and_fixes

c2d8b33

Seb's detekt ruleset, and relevant fixes

rock3r approved these changes Feb 27, 2021

View reviewed changes

rock3r merged commit 8743810 into master Feb 27, 2021

rock3r deleted the 52_detekt branch February 27, 2021 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve detekt findings #54

Resolve detekt findings #54

mpgirro commented Feb 24, 2021 •

edited

Loading

coveralls commented Feb 24, 2021 •

edited

Loading

mpgirro Feb 24, 2021 •

edited

Loading

rock3r Feb 25, 2021

mpgirro Feb 25, 2021

mpgirro Feb 24, 2021

mpgirro Feb 24, 2021

rock3r Feb 25, 2021 •

edited

Loading

mpgirro Feb 25, 2021

rock3r Feb 26, 2021

mpgirro Feb 24, 2021

rock3r Feb 25, 2021

mpgirro Feb 25, 2021

mpgirro Feb 24, 2021

mpgirro Feb 24, 2021 •

edited

Loading

rock3r Feb 25, 2021

mpgirro Feb 24, 2021

rock3r Feb 25, 2021

mpgirro Feb 24, 2021 •

edited

Loading

mpgirro Feb 24, 2021

mpgirro Feb 24, 2021

rock3r Feb 25, 2021

rock3r left a comment

mpgirro commented Feb 26, 2021

rock3r commented Feb 26, 2021

rock3r commented Feb 27, 2021

rock3r commented Feb 27, 2021

mpgirro commented Feb 27, 2021

rock3r commented Feb 27, 2021

		public fun addAllAuthorBuilder(authorBuilders: List<PersonBuilder>): AtomBuilder =
		apply { authorBuilders.forEach(::addAuthorBuilder) }

		"contributor" -> ifCanBeParsed { toPersonBuilder(personBuilderProvider.createPersonBuilder(), namespace) }
		?.let(atomBuilder::addContributorBuilder)

Resolve detekt findings #54

Resolve detekt findings #54

Conversation

mpgirro commented Feb 24, 2021 • edited Loading

coveralls commented Feb 24, 2021 • edited Loading

Pull Request Test Coverage Report for Build #30

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

mpgirro Feb 24, 2021 • 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

rock3r Feb 25, 2021 • 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

mpgirro Feb 24, 2021 • 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

mpgirro Feb 24, 2021 • 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

rock3r left a comment

Choose a reason for hiding this comment

mpgirro commented Feb 26, 2021

rock3r commented Feb 26, 2021

rock3r commented Feb 27, 2021

rock3r commented Feb 27, 2021

mpgirro commented Feb 27, 2021

rock3r commented Feb 27, 2021

mpgirro commented Feb 24, 2021 •

edited

Loading

coveralls commented Feb 24, 2021 •

edited

Loading

mpgirro Feb 24, 2021 •

edited

Loading

rock3r Feb 25, 2021 •

edited

Loading

mpgirro Feb 24, 2021 •

edited

Loading

mpgirro Feb 24, 2021 •

edited

Loading