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

Issue #2944: Changed the logging levels #3491

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

d-ambatenne
Copy link
Contributor

Changed the logging levels according to the new requirements stated in DokkaLogging.kt

Copy link
Collaborator

@whyoleg whyoleg left a comment

Choose a reason for hiding this comment

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

As far as I checked locally, now there is no anymore logging with info level (only in tests) - is it expected?

@@ -15,5 +15,8 @@
<mapping directory="$PROJECT_DIR$/dokka-integration-tests/gradle/projects/coroutines/kotlinx-coroutines" vcs="Git" />
<mapping directory="$PROJECT_DIR$/dokka-integration-tests/gradle/projects/serialization/kotlinx-serialization" vcs="Git" />
<mapping directory="$PROJECT_DIR$/dokka-integration-tests/maven/projects/biojava/biojava" vcs="Git" />
<mapping directory="$PROJECT_DIR$/integration-tests/gradle/projects/coroutines/kotlinx-coroutines" vcs="Git" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you please rebase on master? those VCS mappings are removed there (after some refactoring to integration tests)

processSubmodules()

report("Finishing processing")
report("Finishing processing", LoggingLevel.PROGRESS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

may be it should be debug here?

@@ -81,9 +93,9 @@ private fun timed(logger: DokkaLogger? = null, block: Timer.() -> Unit): Timer =
try {
block()
} catch (exit: GracefulGenerationExit) {
report("Exiting Generation: ${exit.reason}")
report("Exiting Generation: ${exit.reason}", LoggingLevel.PROGRESS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

may be here we can just do logger.progress(exit.reason) and leave report logic fully with debug level, and so no need to have report with custom logging level?
F.e:

try { 
  block()
  report("Exiting Generation")
} catch (exit: GracefulGenerationExit) { 
  logger.progress(exit.reason)
  report("Exiting Generation: ${exit.reason}")
} catch (cause: Throwable) {
  report("Exiting Generation: ${cause.message}")
  throw cause
}

AFAIU report is mostly for debugging purpose

@@ -164,7 +164,7 @@ internal fun topologicalSortByDependantSourceSets(
souceSet.dependentSourceSets.mapNotNull { dependentSourceSetId ->
sourceSets.find { it.sourceSetID == dependentSourceSetId }
// just skip
?: null.also { logger.error("Unknown source set Id $dependentSourceSetId in dependencies of ${souceSet.sourceSetID}") }
?: null.also { logger.warn("Unknown source set Id $dependentSourceSetId in dependencies of ${souceSet.sourceSetID}") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

I will mention it here, but it affects all error->warn changes.
Previously we emitted a lot of errors, but now we report only 3 of them and a lot of warnings. So if project will not set failOnWarning=true there could be a lot of warnings which could affect quality of generated HTML.
I would expect that users who generate documentation will not check that produced HTML is still fine after new declaration (or anything else) is added/removed. Also those warnings could be easily missed when updating project/Dokka, especially if the project documentation is build on CI.
I'm just trying to understand how it will affect our users. May be we need to set failOnWarning=true by default in this case, to prevent users from generation of bad HTML?

Or may be we should better specify what is successful documentation generation? As from DokkaLogger.error documentation:

This level is for logging error messages that describe what prevented Dokka from proceeding with successful documentation generation

What do we mean by successful documentation generation? Does HTML with absent data/declarations is considered successful?

@IgnatBeresnev IgnatBeresnev removed their request for review August 30, 2024 12:32
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.

4 participants