-
Notifications
You must be signed in to change notification settings - Fork 396
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
Expose MARKDOWN_FILE constant #3366
Conversation
/** | ||
* Some parts of Dokka, specifically around [CustomDocTag], depend on this "MARKDOWN_FILE" constant. | ||
* However, it's unclear why exactly, and the chances of breaking something if it gets removed are not | ||
* zero, so it will stay as internal API for now, likely until the core data model is stabilized (#3365). | ||
* | ||
* You can depend on it in your existing code, but please refrain from using it in the new code | ||
* of your plugin unless absolutely necessary. If something does not work without using this constant, | ||
* please report your use case to https://github.com/Kotlin/dokka/issues/3365, it will help a lot. | ||
* | ||
* This constant is not marked with [InternalDokkaApi] on purpose. Even though it is discouraged to use it, | ||
* we understand that some existing code might depend on it, so once a replacement is provided, | ||
* this constant should be deprecated with a message that will help users migrate to something stable. | ||
*/ | ||
public const val MARKDOWN_ELEMENT_FILE_NAME: String = "MARKDOWN_FILE" |
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.
This is the only meaningful change in this PR. Everything else just changes "MARKDOWN_FILE"
literals to use this constant, so that we at least know the places where it is used.
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.
Should it be marked by the InternalDokkaApi
annotation?
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.
The last paragraph in the KDoc has the answer :)
* This constant is not marked with [InternalDokkaApi] on purpose. Even though it is discouraged to use it,
* we understand that some existing code might depend on it, so once a replacement is provided,
* this constant should be deprecated with a message that will help users migrate to something stable.
@@ -8,6 +8,7 @@ plugins { | |||
|
|||
dependencies { | |||
compileOnly(projects.dokkaSubprojects.dokkaCore) | |||
compileOnly(projects.dokkaSubprojects.analysisKotlinApi) |
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.
Is it really a good idea to make java-psi and markdown analysis depend on kotlin-api?
For me it looks really strange...
May be it's better to move this declaration to core
, in internal
package? As we will anyway do something later with it?
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.
To be honest, I don't understand why we can't figure out what it is actually used for in this PR. Instead of it, we expose the internal constant that plugins'll likely depend on. This constant's important as it's like a root name by default. Anyway we will force Dakka and other users to migrate to the new constant again.
We can try the new name (not MARKDOWN_FILE
) here since Dakka should migrate to it anyway. We've already changed the package location. It's unnecessary to use the old name.
And we'll have a possibility to revisit it in #3365
However, there are some dodgy checks like
I think the names of these checks stem from the name of constant. They are local.
and it is mapped using this constant in
You can also see usages MarkdownElementTypes.MARKDOWN_FILE
in the markdown artifact (or ask it's team). This is just the name of the root element.
/** | ||
* Some parts of Dokka, specifically around [CustomDocTag], depend on this "MARKDOWN_FILE" constant. | ||
* However, it's unclear why exactly, and the chances of breaking something if it gets removed are not | ||
* zero, so it will stay as internal API for now, likely until the core data model is stabilized (#3365). | ||
* | ||
* You can depend on it in your existing code, but please refrain from using it in the new code | ||
* of your plugin unless absolutely necessary. If something does not work without using this constant, | ||
* please report your use case to https://github.com/Kotlin/dokka/issues/3365, it will help a lot. | ||
* | ||
* This constant is not marked with [InternalDokkaApi] on purpose. Even though it is discouraged to use it, | ||
* we understand that some existing code might depend on it, so once a replacement is provided, | ||
* this constant should be deprecated with a message that will help users migrate to something stable. | ||
*/ | ||
public const val MARKDOWN_ELEMENT_FILE_NAME: String = "MARKDOWN_FILE" |
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.
Should it be marked by the InternalDokkaApi
annotation?
A few results from the investigation on what is Its usage was first introduced in this commit ec63795 of this PR #1473 to fix #1458. The idea behind it is to have some kind of ROOT tag for documentation blocks to remove nesting Parsing documentation happens in So, in the end looks like it's just root tag, which for some reason is not the different class (like BTW, I've added |
Filled #3404 based on comment above |
e3dcd94
to
d20a6db
Compare
I've rebased this PR onto master, and updated the KDoc to include the research results from above, along with a link to the issue: 64a9b09 I contemplated extracting this constant into With a normal-looking name and inside I feel like it's best to address this properly as part of #3404, and have this PR serve as an enabler for Dackka and be a tiny incremental improvement. I've also moved it out of the |
62001dc
to
64a9b09
Compare
Part of #3112, since Dackka depends on it for creating a few custom tags.
The story with this constant is the following. There was a constant in the
org.jetbrains.markdown
library:org.intellij.markdown.MarkdownElementTypes.MARKDOWN_FILE.name
whose value wasMARKDOWN_FILE
.It was used as the
name
ofCustomDocTag
, and in a few other places.Because Dokka used to expose PSI, kotlin-compiler and other internal libs as
api
dependencies, some downstream plugins (like Dackka) started using it as well, because they saw how Dokka used it and they just (allegedly) copied it.When the analysis refactoring happened in #3034, all of these internal dependencies were no longer exposed to the user, so the code that was using
org.intellij.markdown.MarkdownElementTypes.MARKDOWN_FILE.name
couldn't be compiled anymore.I looked at the use cases, but I couldn't figure out why it was needed in the first place. I mean, sure, it's used in
CustomDocTag
as the defaultname
, so we should just rename it and call itCUSTOM_DOC_TAG_DEFAULT_NAME
, right?However, there are some dodgy checks like
dokka/dokka-subprojects/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/transformers/pages/comments/DocTagToContentConverter.kt
Line 269 in 5bb1c8a
and
dokka/dokka-subprojects/analysis-java-psi/src/main/kotlin/org/jetbrains/dokka/analysis/java/parsers/JavaDocCommentParser.kt
Line 203 in 5bb1c8a
and it is mapped using this constant in
dokka/dokka-subprojects/analysis-markdown-jb/src/main/kotlin/org/jetbrains/dokka/analysis/markdown/jb/factories/DocTagsFromIElementFactory.kt
Line 61 in 5bb1c8a
So it makes me question whether
CustomDocTag
is used not only for custom tags, but also for something to do with markdown files?To not make any rushed decisions that will need to be revisited later on anyway, I decided to expose this constant as it is, with an honest KDoc. We'll need to figure out what it was actually used for, and come up with an alternative, most likely when we start stabilizing the core API and its data model. Created an issue to keep track of it: #3365