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

Refactor and document DisplaySourceSet #3105

Merged
merged 7 commits into from
Aug 14, 2023
Merged

Conversation

qwwdfsad
Copy link
Contributor

@qwwdfsad qwwdfsad commented Aug 4, 2023

  • Decommission SelfRepresentingSingletonSet as harmful
  • Document the purpose and the semantics of DisplaySourceSet
  • Slightly refactor code around

…ing harmful and unimplement it in DisplaySourceSet

* Provide no automatic migration for DisplaySourceSet as there is no mechanisms for that. Manual migration is replacement of 'dss' to `setOf(dss)` where applicable
* Introduce a convenience-member DefaultRenderer.buildContentNode to avoid wrapping DSS into set manually

Fixes #2897
@qwwdfsad qwwdfsad changed the title Refactor display source set Refactor and document DisplaySourceSet Aug 4, 2023
*/
data class DisplaySourceSet(
public data class DisplaySourceSet(
Copy link
Contributor Author

@qwwdfsad qwwdfsad Aug 4, 2023

Choose a reason for hiding this comment

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

IMO it is worth making all constructors internal and leaving only DokkaSourceSet.toDisplaySourceSet. The API already kind of dictates that, but I'm not sure how drastic this change will be

Copy link
Member

Choose a reason for hiding this comment

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

I am wondering about a case with multiple DokkaSourceSet (when we really need the primary constructor and CompositeSourceSetID). I found a such case only in tests and do not know about an external case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably worth figuring it out systematically. This one might be too strict, especially for 1.9.20

…ward Iterable<DisplaySourceSet>.computeSourceSetIds()

* Refactor all the usages, save some allocations
* Start caching CompositeSourceSetID properties to avoid excessive allocations
@qwwdfsad qwwdfsad force-pushed the refactor-display-source-set branch from c20a1f3 to 116d63d Compare August 4, 2023 17:16
@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Aug 4, 2023

The integration test failure is related to pathsaver plugin in Knit -- I'll see if it's patchable locally

qwwdfsad added a commit to Kotlin/kotlinx.coroutines that referenced this pull request Aug 6, 2023
Into the master because the update contains a workaround required for Dokka, see Kotlin/dokka#3105
qwwdfsad added a commit to Kotlin/kotlinx.coroutines that referenced this pull request Aug 6, 2023
Into the master because the update contains a workaround required for Dokka, see Kotlin/dokka#3105
qwwdfsad added a commit to Kotlin/kotlinx.serialization that referenced this pull request Aug 6, 2023
Into the master because the update contains a workaround required for Dokka, see Kotlin/dokka#3105
@qwwdfsad
Copy link
Contributor Author

qwwdfsad commented Aug 7, 2023

I've patched the Knit, updated it in coroutines/serialization and checked it helps. Next step is to update the submodule

qwwdfsad added a commit to Kotlin/kotlinx.serialization that referenced this pull request Aug 7, 2023
Into the master because the update contains a workaround required for Dokka, see Kotlin/dokka#3105
@qwwdfsad qwwdfsad requested a review from vmishenev August 7, 2023 15:08
@IgnatBeresnev IgnatBeresnev linked an issue Aug 11, 2023 that may be closed by this pull request
core/src/main/kotlin/model/DisplaySourceSet.kt Outdated Show resolved Hide resolved
*/
data class DisplaySourceSet(
public data class DisplaySourceSet(
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering about a case with multiple DokkaSourceSet (when we really need the primary constructor and CompositeSourceSetID). I found a such case only in tests and do not know about an external case.

@qwwdfsad qwwdfsad merged commit 2269ac5 into master Aug 14, 2023
9 checks passed
@qwwdfsad qwwdfsad deleted the refactor-display-source-set branch August 14, 2023 17:45
dkhalanskyjb pushed a commit to Kotlin/kotlinx.coroutines that referenced this pull request Dec 5, 2023
Into the master because the update contains a workaround required for Dokka, see Kotlin/dokka#3105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor DisplaySourceSet
2 participants