Add matchingRegex #598

martinbonnin · 2020-01-27T15:06:02Z

Closes #593

This allows to match a package based on a regex and not only a prefix.

            perPackageOption {
                matchingRegex = ".*\\.internal"
                suppress = true
            }

core/src/main/kotlin/DokkaBootstrapImpl.kt

MarcinAman · 2020-10-12T15:27:54Z

Hi, since this PR was created a lot of dokka has changed. Would you mind rebasing it onto master?

martinbonnin · 2020-10-12T15:45:36Z

Hi, since this PR was created a lot of dokka has changed. Would you mind rebasing it onto master?

Yup, give me a bit of time to switch back to this and I'll rebase.

MarcinAman · 2020-10-13T05:02:06Z

Right now dokka has a prefix option in PackageOptions. I think it would be worth to change that to your regex-based approach to make it more powerfull, i dont feel like we need 2 options to configure more or less same thing

martinbonnin · 2020-11-11T15:04:04Z

@MarcinAman PR is rebased. Removing prefix is technically a breaking change, let me know if that can be shipped at this stage or if you want me to add it again with a deprecation warning (which will complexify both the documentation and the handling of default values but hey, it happens!)

martinbonnin · 2020-11-11T15:05:34Z

core/src/main/kotlin/transformers/documentation/PreMergeDocumentableTransformer.kt

@@ -28,8 +28,8 @@ fun PreMergeDocumentableTransformer.sourceSet(documentable: Documentable): Dokka
 fun PreMergeDocumentableTransformer.perPackageOptions(documentable: Documentable): PackageOptions? {
    val packageName = documentable.dri.packageName ?: return null
    return sourceSet(documentable).perPackageOptions
-        .sortedByDescending { packageOptions -> packageOptions.prefix.length }
-        .firstOrNull { packageOptions -> packageName.startsWith(packageOptions.prefix) }
+        .sortedByDescending { packageOptions -> packageOptions.matchingRegex.length }


This feels a bit awkward with regexes. I think I would prefer the order of the rules in the Gradle file to determine priority but I'll let people more familiar with the subject decide.

@kamildoleglo ?

I think ordering in Gradle might be more problematic and potentially could break easier. Also it will cause some confusion especially when having split logic for dokka configuration (eg. some packageOptions in buildSrc and some in Gradle build files), so I'd personally stick with the regex for now

Fair point about split logic, especially since it's hard to know in advance the order of execution of Gradle scripts. I added a small note to the Gradle documentation to tell users that the longuest matchingRegexp will be used.

For the cli usage, maybe the order of arguments would still make sense? Or ultimately, maybe an explicit priority option will be needed. But this can be all be added later depending the needs and use cases

MarcinAman · 2020-11-12T17:53:53Z

@MarcinAman PR is rebased. Removing prefix is technically a breaking change, let me know if that can be shipped at this stage or if you want me to add it again with a deprecation warning (which will complexify both the documentation and the handling of default values but hey, it happens!)

It can be shipped, as for now Dokka doesn't have any guarantees about stability.

Thanks, i'll check out the changes tomorrow 👍

MarcinAman · 2020-11-13T17:47:44Z

core/src/main/kotlin/transformers/documentation/PreMergeDocumentableTransformer.kt

@@ -28,8 +28,8 @@ fun PreMergeDocumentableTransformer.sourceSet(documentable: Documentable): Dokka
 fun PreMergeDocumentableTransformer.perPackageOptions(documentable: Documentable): PackageOptions? {
    val packageName = documentable.dri.packageName ?: return null
    return sourceSet(documentable).perPackageOptions
-        .sortedByDescending { packageOptions -> packageOptions.prefix.length }
-        .firstOrNull { packageOptions -> packageName.startsWith(packageOptions.prefix) }
+        .sortedByDescending { packageOptions -> packageOptions.matchingRegex.length }


@kamildoleglo ?

kamildoleglo

Just one request to remove the unnecessary (I believe) check and it should be good to go

kamildoleglo · 2020-11-16T11:25:31Z

core/src/main/kotlin/DokkaBootstrapImpl.kt

@@ -7,8 +7,8 @@ import java.util.function.BiConsumer


 fun parsePerPackageOptions(args: List<String>): List<PackageOptions> = args.map { it.split(",") }.map {


I think this check is now unnecessary as we dropped global settings and I see no reason why the regex could not be an all match

kamildoleglo · 2020-11-16T11:28:40Z

core/src/main/kotlin/transformers/documentation/PreMergeDocumentableTransformer.kt

@@ -28,8 +28,8 @@ fun PreMergeDocumentableTransformer.sourceSet(documentable: Documentable): Dokka
 fun PreMergeDocumentableTransformer.perPackageOptions(documentable: Documentable): PackageOptions? {
    val packageName = documentable.dri.packageName ?: return null
    return sourceSet(documentable).perPackageOptions
-        .sortedByDescending { packageOptions -> packageOptions.prefix.length }
-        .firstOrNull { packageOptions -> packageName.startsWith(packageOptions.prefix) }
+        .sortedByDescending { packageOptions -> packageOptions.matchingRegex.length }


I think ordering in Gradle might be more problematic and potentially could break easier. Also it will cause some confusion especially when having split logic for dokka configuration (eg. some packageOptions in buildSrc and some in Gradle build files), so I'd personally stick with the regex for now

MarcinAman · 2020-11-18T11:48:13Z

Thanks ☺️

MarcinAman · 2020-11-18T12:11:29Z

You can check out the build with your changes by adding a new repository: maven("https://maven.pkg.jetbrains.space/kotlin/p/dokka/dev") and updating version to: 1.4.20-dev-23

* add matchingRegex as a simpler replacement for `prefix` * remove useless check * added a note about the order of the matchingRegex

martinbonnin mentioned this pull request Jan 27, 2020

Dokka 0.10.0 ignore unsuppresed packages in suppressed packages #521

Closed

Kordyjan requested a review from kamildoleglo January 28, 2020 09:25

kamildoleglo reviewed Jan 28, 2020

View reviewed changes

core/src/main/kotlin/DokkaBootstrapImpl.kt Outdated Show resolved Hide resolved

martinbonnin mentioned this pull request Jan 28, 2020

Add a CONTRIBUTING.md #597

Merged

add matchingRegex as a simpler replacement for prefix

a5081a8

martinbonnin force-pushed the add-matchingRegex branch from e85a18c to a5081a8 Compare November 11, 2020 15:02

martinbonnin changed the base branch from dev-0.10.1 to master November 11, 2020 15:02

martinbonnin commented Nov 11, 2020

View reviewed changes

MarcinAman requested review from kamildoleglo, MarcinAman and sellophane November 12, 2020 17:54

MarcinAman approved these changes Nov 13, 2020

View reviewed changes

kamildoleglo suggested changes Nov 16, 2020

View reviewed changes

martinbonnin added 2 commits November 16, 2020 12:51

remove useless check

77729f4

added a note about the order of the matchingRegex

f97135b

kamildoleglo approved these changes Nov 16, 2020

View reviewed changes

MarcinAman merged commit 1959b91 into Kotlin:master Nov 18, 2020

MarcinAman pushed a commit that referenced this pull request Nov 26, 2020

Add matchingRegex (#598)

f944142

* add matchingRegex as a simpler replacement for `prefix` * remove useless check * added a note about the order of the matchingRegex

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add matchingRegex #598

Add matchingRegex #598

martinbonnin commented Jan 27, 2020

MarcinAman commented Oct 12, 2020

martinbonnin commented Oct 12, 2020

MarcinAman commented Oct 13, 2020

martinbonnin commented Nov 11, 2020

martinbonnin Nov 11, 2020

MarcinAman Nov 13, 2020

kamildoleglo Nov 16, 2020

martinbonnin Nov 16, 2020

MarcinAman commented Nov 12, 2020

MarcinAman Nov 13, 2020

kamildoleglo left a comment

kamildoleglo Nov 16, 2020

kamildoleglo Nov 16, 2020

MarcinAman commented Nov 18, 2020

MarcinAman commented Nov 18, 2020

		@@ -7,8 +7,8 @@ import java.util.function.BiConsumer


		fun parsePerPackageOptions(args: List<String>): List<PackageOptions> = args.map { it.split(",") }.map {

Add matchingRegex #598

Add matchingRegex #598

Conversation

martinbonnin commented Jan 27, 2020

MarcinAman commented Oct 12, 2020

martinbonnin commented Oct 12, 2020

MarcinAman commented Oct 13, 2020

martinbonnin commented Nov 11, 2020

martinbonnin Nov 11, 2020

Choose a reason for hiding this comment

MarcinAman Nov 13, 2020

Choose a reason for hiding this comment

kamildoleglo Nov 16, 2020

Choose a reason for hiding this comment

martinbonnin Nov 16, 2020

Choose a reason for hiding this comment

MarcinAman commented Nov 12, 2020

MarcinAman Nov 13, 2020

Choose a reason for hiding this comment

kamildoleglo left a comment

Choose a reason for hiding this comment

kamildoleglo Nov 16, 2020

Choose a reason for hiding this comment

kamildoleglo Nov 16, 2020

Choose a reason for hiding this comment

MarcinAman commented Nov 18, 2020

MarcinAman commented Nov 18, 2020