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

[K2] Support kotlin-as-java and javadoc plugins and update version of Analysis API #3227

Merged
merged 8 commits into from Oct 26, 2023

Conversation

vmishenev
Copy link
Member

@vmishenev vmishenev commented Oct 17, 2023

Also, it contains some fixes to make unit tests of kotlin-as-java and javadoc plugins green.

@vmishenev vmishenev linked an issue Oct 17, 2023 that may be closed by this pull request
@vmishenev vmishenev changed the title Implement InheritanceBuilder and for symbols Support kotlin-as-java and javadoc plugins Oct 17, 2023
@vmishenev vmishenev self-assigned this Oct 17, 2023
@vmishenev vmishenev force-pushed the 3135-Support-kotlin-as-java-and-javadoc-plugins branch 2 times, most recently from 88f4b62 to d30770d Compare October 17, 2023 17:01
@vmishenev vmishenev requested review from IgnatBeresnev and removed request for IgnatBeresnev October 17, 2023 17:17
@vmishenev vmishenev marked this pull request as ready for review October 17, 2023 17:30
/**
* This is copy-pasted from org.jetbrains.dokka.analysis.kotlin.descriptors.compiler.impl.DescriptorInheritanceBuilder and adapted for symbols
*/
internal class SymbolInheritanceBuilder(context: DokkaContext) : InheritanceBuilder {
Copy link
Member Author

Choose a reason for hiding this comment

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

There is no way to verify it due to #3232

Comment on lines 58 to 72
// In K2 name of accessors parameter is `value`
assert(
setOf(
"getIssuesFetched()",
"setIssuesFetched(Integer issuesFetched)",
"isFoo()",
"setFoo(String isFoo)",
) == props || setOf(
"getIssuesFetched()",
"setIssuesFetched(Integer value)",
"isFoo()",
"setFoo(String value)",
) == props
)
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to stick to asserts from kotlin.test

And what do you think about introducing a tag/annotation similar to OnlyDescriptors, but for Symbols? Then we could copy-paste this test with two different expected values, and mark one as only for descriptors and the other as only for symbols. This way, it will be easier to find such cases in the future and remove them once K1 is dropped, otherwise this assert will stay like this forever :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's try to stick to asserts from kotlin.test

Sorry, I have missed it(

And what do you think about introducing a tag/annotation similar to OnlyDescriptors, but for Symbols?

I thought about it. But

  • I do not want to encourage to make the difference in documentable model between K1 and K2.
  • Currently, we have only 2 such tests (this test and another 'catch exception'), where we have 2 possibilities. K1 works correctly for all out tests except [K1] Incorrect Java synthetic properties #3128 If we have new tests where K1 works incorrectly, it will make sense to add 'OnlyDescriptors'. Or we will decide it during Fix inconsistencies between K1 and K2 #3115

Copy link
Member

Choose a reason for hiding this comment

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

Just make sure you don't forget about it, as this test is not marked with any annotation and there's no TODO. In a year or so, it will be difficult for other people to understand why this assert is written this way

@vmishenev vmishenev changed the title Support kotlin-as-java and javadoc plugins [K2] Support kotlin-as-java and javadoc plugins Oct 18, 2023
@vmishenev vmishenev added the topic: K2 Issues / PRs that are related to the K2 migration. See #2888 label Oct 18, 2023
@vmishenev vmishenev changed the title [K2] Support kotlin-as-java and javadoc plugins [K2] Support kotlin-as-java and javadoc plugins and update version of Analysis API Oct 19, 2023
Comment on lines 112 to 113
internal val descriptorInheritanceBuilder by extending {
plugin<InternalKotlinAnalysisPlugin>().inheritanceBuilder providing ::SymbolInheritanceBuilder
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
internal val descriptorInheritanceBuilder by extending {
plugin<InternalKotlinAnalysisPlugin>().inheritanceBuilder providing ::SymbolInheritanceBuilder
internal val symbolInheritanceBuilder by extending {
plugin<InternalKotlinAnalysisPlugin>().inheritanceBuilder providing ::SymbolInheritanceBuilder

@vmishenev vmishenev force-pushed the 3135-Support-kotlin-as-java-and-javadoc-plugins branch 2 times, most recently from 6399b1d to f6f45ca Compare October 24, 2023 17:04
@vmishenev vmishenev force-pushed the 3135-Support-kotlin-as-java-and-javadoc-plugins branch from f6f45ca to 545d921 Compare October 25, 2023 17:07
@vmishenev vmishenev merged commit 9b07435 into master Oct 26, 2023
26 checks passed
@vmishenev vmishenev deleted the 3135-Support-kotlin-as-java-and-javadoc-plugins branch October 26, 2023 11:39
@IgnatBeresnev IgnatBeresnev added this to the Dokka 1.9.20 milestone Dec 21, 2023
@IgnatBeresnev IgnatBeresnev removed this from the Dokka 1.9.20 milestone Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: K2 Issues / PRs that are related to the K2 migration. See #2888
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[K2] Support kotlin-as-java and javadoc plugins
2 participants