-
Notifications
You must be signed in to change notification settings - Fork 471
Add 'name' property to TypeParam and use it in TypeReference.toJavadocURL
#4344
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 'name' property to TypeParam and use it in TypeReference.toJavadocURL
#4344
Conversation
dokka-subprojects/core/src/main/kotlin/org/jetbrains/dokka/links/DRI.kt
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public data class TypeParam(val bounds: List<TypeReference>) : TypeReference() | ||
| public data class TypeParam(val name: String, val bounds: List<TypeReference>) : TypeReference() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, we should also add toString here?
In any case, we again broke DRI.toString with these changes.
Something like <$name:${bounds.joinToString(",")}>?
< and > might be needed here to distinguish type parameters from other type references.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not like that we are breaking package-list. Unlike the previous case, this could break many links.
WDYT about replacing DRI.toString() with a DRI renderer that keeps the old format in DefaultExternalLocationProvider and PackageListService?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added custom rendered. We can now, if we want, render varargs as kotlin.Array[*] again, to keep compatibility. Am not sure if this is a good idea though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if a custom renderer is a good idea. Now we need to update things in two places. For example, #4347 also changes DRI.toString and so now we need to update toUrlString to be consistent :)
Additionally, I've decided to review a couple of projects package-list files: all projects available on kotlinlang API page, android reference, skiko, ktor, okio, and okhttp. (all checked files package-lists.zip)
And we do print DRI there for two reasons:
- In case of single module projects (like
kotlinx-datetime,skiko, orokio) - because in that case we, for some reason, generate paths, which are not "default" ones for our package-list renderer and resolver. - In case we have declarations with the same FQN, but in different source sets - in
ktorforjsengine, which is shared betweenjsandwasm-jstargets, but still have some exposed internals (publicdeclarations) in corresponding source-sets.
And we do have TypeParam usages there:
- In
ktor- but those areinternals, so we don't care - In
kotlinx.datetime- there are some usages informatAPI, not sure if there will beexternal usagesin libraries for these types, + they will migrate to multi module build and the package-list will not contain those - There are 12 usages in
okioand 4 usages inskiko. In most cases, those are functions likeuse/read/write.
So, in the end, I don't think that we will break many links if we change the output of TypeParam.toString, as this only affects package-list compatibility.
What I wanted to say is that maybe it's fine to break those links for the sake of consistency. Or, if you think that it's still not a good idea to break them, at least continue to use DRI.toString everywhere and just update TypeParam.toString to be compatible with previous versions (e.g., don't include name there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, #4347 also changes DRI.toString and so now we need to update toUrlString to be consistent :)
Do we really need to change it? I'm not going to change it until it breaks something.
By the way, some time ago, it was decided not to rely on .toString and have a mapping logic instead, since
.toStringcan lead to bugs such as Strange internal reference IDs in HTML #2803- Non-obvious that
.toStringis reused in the HTML/package-listrenderers directly. From certain perspectives, the Documentable model should not contain rendering logic.
It seems that leaking DRI to HTML/package-list was made by mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to change it? I'm not going to change it until it breaks something.
I'm not sure I understand. If we don't change it for properties, then we might have an issue where some links will be incorrectly resolved for properties from the package list. Or am I missing something here?
It seems that leaking DRI to HTML/package-list was made by mistake.
Yes, true, and I would expect it might be leaked in more places. That's why I'm a bit concerned that introducing an additional toUrlString with different behavior might do more harm than good at this point.
And so at this point, I would say it will be safer to rely on DRI.toString.
From certain perspectives, the Documentable model should not contain rendering logic.
Yes, but in this case, I think it's not about rendering per se, but rather about some constant identifier, and in this case, DRI and its DRI.toString is this identifier, and it should be fine to rely on it in rendering.
At this point, I would really suggest that we just update TypeParam.toString to return the same string as before ("TypeParam(bounds=[$bounds])", without name property) and discuss DRI.toString usage in HTML/package-list separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that is the best resolution in terms of the current PR.
I will create a separate issue for the usage of DRI.toString in HTML/package-lists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might have an issue where some links will be incorrectly resolved for properties from the package list. Or am I missing something here?
If we have .toUrlString , we won’t have any issues resolving properties from the package list.
Since we decided to keep DRI.toString, I am going to restore the old version of DRI.toString in #4347
DRI and its DRI.toString is this identifier, and it should be fine to rely on it in rendering.
This is debatable. There are different points of view. For example, the previous one is not mine)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b2ff795 to
546d86c
Compare
ffdf8e0 to
2cad18d
Compare
Fix for #3502