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

Innovation 2022Q4 - Add a github Wiki #1169

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

Conversation

xgouchet
Copy link
Collaborator

@xgouchet xgouchet commented Dec 1, 2022

What does this PR do?

This PR adds an automated way to generate a global api reference documentation in the form of a Github Wiki

Additional Notes

The solution uses 3 steps :

  • Use the existing Dokka integration to generate (Github flavored) Markdown documentation
  • Use a custom gradle plugin to generate markdown files in a simpler hierarchy that is compatible with a Github Wiki repo
  • Use a python script to update the wiki repository (triggered by a gitlab job whenever something is updated on the master branch)

@xgouchet xgouchet requested review from a team as code owners December 1, 2022 14:58
@xgouchet xgouchet changed the base branch from develop to master December 1, 2022 14:59
@xgouchet xgouchet removed the request for review from a team December 1, 2022 14:59
0xnm
0xnm previously approved these changes Dec 1, 2022
Copy link
Contributor

@0xnm 0xnm left a comment

Choose a reason for hiding this comment

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

Very nice addition to the project! I've added some comments, they are non-blocking for me.

Comment on lines 22 to 27
val task = target.tasks.register(GEN_TASK_NAME, GenerateWikiTask::class.java) {
this.srcDir = File(File(File(target.buildDir, "reports"), "javadoc"), target.name)
this.apiSurface = File(target.projectDir, "apiSurface")
this.outputDir = File(target.buildDir, "wiki")
this.projectName = target.name
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there is no need to use target.afterEvaluate and get task explicitly. With lazy API here it should be possible to declare task dependencies like the following:

Suggested change
val task = target.tasks.register(GEN_TASK_NAME, GenerateWikiTask::class.java) {
this.srcDir = File(File(File(target.buildDir, "reports"), "javadoc"), target.name)
this.apiSurface = File(target.projectDir, "apiSurface")
this.outputDir = File(target.buildDir, "wiki")
this.projectName = target.name
}
val task = target.tasks.register(GEN_TASK_NAME, GenerateWikiTask::class.java) {
this.srcDir = File(File(File(target.buildDir, "reports"), "javadoc"), target.name)
this.apiSurface = File(target.projectDir, "apiSurface")
this.outputDir = File(target.buildDir, "wiki")
this.projectName = target.name
dependsOn("dokkaGfm")
dependsOn(ApiSurfacePlugin.TASK_GEN_API_SURFACE)
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea

githubwiki.py Outdated
import requests
from git import Repo

OWNER = "xgouchet" # "DataDog"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be replaced before the merge

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, done

githubwiki.py Outdated
def git_push_changes(repo: Repo):
print("Pushing branch")
origin = repo.remote(name="origin")
repo.git.push("--set-upstream", "--force", origin, repo.head.ref)
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need --force here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed we might not need it

Comment on lines 74 to 79
if (typeDir.exists()) {
println("Combining doc from $typeCanonicalName")
combine(typeDir, outputFile, typeName)
} else {
System.err.println("Unable to find $typeDir")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it is better to use logger property here (and in other places) for logging

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, done

@codecov-commenter
Copy link

codecov-commenter commented Dec 2, 2022

Codecov Report

Merging #1169 (e5ed81c) into master (d2caeca) will decrease coverage by 0.06%.
The diff coverage is n/a.

❗ Current head e5ed81c differs from pull request most recent head 0518ad5. Consider uploading reports for the commit 0518ad5 to get more accurate results

@@            Coverage Diff             @@
##           master    #1169      +/-   ##
==========================================
- Coverage   83.19%   83.13%   -0.06%     
==========================================
  Files         274      274              
  Lines        9458     9458              
  Branches     1540     1540              
==========================================
- Hits         7868     7862       -6     
  Misses       1153     1153              
- Partials      437      443       +6     
Impacted Files Coverage Δ
...android/log/internal/logger/TelemetryLogHandler.kt 75.00% <0.00%> (-25.00%) ⬇️
...android/rum/internal/ndk/DatadogNdkCrashHandler.kt 85.95% <0.00%> (-2.70%) ⬇️
.../android/rum/internal/monitor/DatadogRumMonitor.kt 92.78% <0.00%> (-0.56%) ⬇️
...ndroid/telemetry/internal/TelemetryEventHandler.kt 73.21% <0.00%> (+0.89%) ⬆️

mariusc83
mariusc83 previously approved these changes Dec 5, 2022
Copy link
Collaborator

@mariusc83 mariusc83 left a comment

Choose a reason for hiding this comment

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

Nice work !!

@xgouchet xgouchet dismissed stale reviews from mariusc83 and 0xnm via 0518ad5 August 7, 2023 06:56
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.

None yet

4 participants