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

Add a distinct divider between function overloads #2585

Merged
merged 1 commit into from Aug 5, 2022

Conversation

IgnatBeresnev
Copy link
Member

@IgnatBeresnev IgnatBeresnev commented Jul 25, 2022

By far not the best solution, but I found no other reliable way to add a divider for function overloads especially considering different sourcesets. This adds no new public API, so I think it's not a big deal - will probably have to redesign this down the road anyway.

See #2576

For function overloads with some description

2022-07-25_20-25-05


For function overloads with the same description

2022-07-25_20-25-25

@IgnatBeresnev IgnatBeresnev mentioned this pull request Jul 25, 2022
Comment on lines -771 to -777
private val String.isAbsolute: Boolean
get() = URI(this).isAbsolute

Copy link
Member Author

Choose a reason for hiding this comment

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

Unused

@@ -321,6 +313,40 @@ open class HtmlRenderer(
}
}

private fun groupDivergentInstancesWithSourceSet(
Copy link
Member Author

Choose a reason for hiding this comment

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

This made buildDivergent considerably longer, even though this particular function needed no context from it and shared nothing in common, so I don't see a reason why it has to be a nested function.

Extracted for the sake of readability of buildDivergent

@IgnatBeresnev
Copy link
Member Author

Proposing to go with this solution as it adds no new public API, so it will be easy to remove when the page is redesigned, which is just a matter of time.

I'll post the final result to #kotlin-libs and ask for feedback. If this version is not liked for some reason, it can be changed before release

@IgnatBeresnev IgnatBeresnev marked this pull request as ready for review July 26, 2022 16:31
Copy link
Member

@vmishenev vmishenev left a comment

Choose a reason for hiding this comment

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

  1. https://dokka-snapshots.s3.eu-central-1.amazonaws.com/fuction-overloads-dinstinct/serialization/8779f50/kotlinx-serialization-core/kotlinx.serialization.builtins/serializer.html
    There are not overloads.
  2. Regression: There is extra space in a declaration list. It requires a check node.dci.kind == ContentKind.Main that it is removed in this PR.

image

@IgnatBeresnev
Copy link
Member Author

IgnatBeresnev commented Jul 27, 2022

There are not overloads.

Not sure what you mean. These are different functions with different description, but with the same function name, so for me it makes an overload 🤔

Fixed the problem with node.dci.kind == ContentKind.Main and left a comment of warning

@vmishenev
Copy link
Member

vmishenev commented Jul 27, 2022

Not sure what you mean. These are different functions with different description, but with the same function name, so for me it makes an overload thinking

Sorry( I wrote a wrong sentence (and a link).

For https://dokka-snapshots.s3.eu-central-1.amazonaws.com/fuction-overloads-dinstinct/stdlib/4ce210b/kotlin-stdlib/kotlin.collections/indices.html, that has several extension properties, it does not have a divider.

@IgnatBeresnev
Copy link
Member Author

IgnatBeresnev commented Jul 28, 2022

has several extension properties

Silly me, for some reason I thought class named Same**Method**NamePageMergerStrategy only merged methods 🥲

Previously there was a check that it was only functions, changed it to accept all members, so both functions and properties. Should be OK now

@vmishenev vmishenev linked an issue Jul 28, 2022 that may be closed by this pull request
@IgnatBeresnev
Copy link
Member Author

After some discussion, we decided to drop "Overload" header and just stick with a simple horizontal line

Updated the PR, @vmishenev please have a look

@IgnatBeresnev IgnatBeresnev merged commit efed96e into master Aug 5, 2022
@IgnatBeresnev IgnatBeresnev deleted the fuction-overloads-dinstinct branch August 5, 2022 11:25
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.

Documentation of function with overloads is hard to distinguish
2 participants