-
Notifications
You must be signed in to change notification settings - Fork 504
MW-213 Discover and implement kotlin docs and generate documentation #1861
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
base: development
Are you sure you want to change the base?
Conversation
|
Good Job @rohitlokhande47 |
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.
Copilot reviewed 2 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (5)
- .run/mifospay-ios.run.xml: Language not supported
- feature/accounts/build.gradle.kts: Language not supported
- feature/accounts/src/commonMain/kotlin/org/mifospay/feature/accounts/AccountViewModel.kt: Language not supported
- feature/auth/build.gradle.kts: Language not supported
- gradle.properties: Language not supported
feature/accounts/build.gradle.kts
Outdated
| } | ||
| } | ||
| // Configure Dokka | ||
| tasks.withType<org.jetbrains.dokka.gradle.DokkaTask>().configureEach { |
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 should be be part of feature and library gradle custom plugin
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.
Move these in feature and library build-logic gradle plugin
feature/auth/build.gradle.kts
Outdated
| } | ||
| } | ||
|
|
||
| tasks.withType<org.jetbrains.dokka.gradle.DokkaTask>().configureEach { |
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.
Same as above, it should be moved in feature and library gradle plugin
gradle/libs.versions.toml
Outdated
| spotless = { id = "com.diffplug.spotless", version.ref = "spotlessVersion" } | ||
| version-catalog-linter = { id = "io.github.pemistahl.version-catalog-linter", version.ref = "versionCatalogLinterVersion" } | ||
|
|
||
| dokka = { id = "org.jetbrains.dokka", version = "2.0.0" } |
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.
use a variable for version number of this library like above libraries instead of hardcoding
gradle.properties
Outdated
|
|
||
| # Enable configuration caching between builds. | ||
| org.gradle.configuration-cache=true | ||
| org.gradle.configuration-cache=false |
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.
Revert it back
feature/auth/build.gradle.kts
Outdated
| } | ||
| } | ||
|
|
||
| tasks.withType<org.jetbrains.dokka.gradle.DokkaTask>().configureEach { |
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.
Move the Dokka configurations into the root build.gradle.kts file and include the Dokka plugin into the existing KMP Library and CMP Feature Convention plugin. and It will be applied to all the core and feature modules and no need to add the plugin on every module and it will works as expected.
…ion popUpTo references
…ion popUpTo references
niyajali
left a comment
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.
Follow this to set up dokka plugin - https://kotlinlang.org/docs/dokka-gradle.html#multi-project-configuration
| override fun apply(target: Project) { | ||
| with(target) { | ||
| with(pluginManager) { | ||
| apply(KMPLibraryPlugin::class.java) |
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.
Why are you adding this?
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.
ok
|
|
||
| plugins { | ||
| `kotlin-dsl` | ||
| alias(libs.plugins.dokka) |
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.
remove this
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.
ok
| compileOnly(libs.ktlint.gradlePlugin) | ||
| compileOnly(libs.spotless.gradle) | ||
| implementation(libs.truth) | ||
| implementation(libs.dokka.gradle.plugin) |
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.
use compileOnly
| implementationClass = "MifosGitHooksConventionPlugin" | ||
| description = "Installs git hooks for the project" | ||
| } | ||
| register("featureLibrary") { |
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.
Remove this plugin we no need such plugin for dokka
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.
ok
| apply("org.jetbrains.kotlin.plugin.compose") | ||
| apply("org.jetbrains.compose") | ||
| apply("org.jetbrains.dokka") | ||
| apply(KMPLibraryPlugin::class.java) |
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.
Remove this plugin apply(KMPLibraryPlugin::class.java)
| viewModel.logOut() | ||
| navController.navigate(LOGIN_GRAPH) { | ||
| popUpTo(navController.graph.id) { | ||
| popUpTo(LOGIN_GRAPH) { |
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.
Why are you changing this.. you're about to configure the dokka plugin for documentation, please revert this & below files as it is
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 will make changes
| apply("mifospay.kmp.koin") | ||
| apply("mifos.detekt.plugin") | ||
| apply("mifos.spotless.plugin") | ||
| apply("org.jetbrains.dokka") |
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 sufficient for docs generation
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.
ok I will look into it
| import org.gradle.api.Project | ||
| import org.gradle.kotlin.dsl.withType | ||
|
|
||
| class KMPLibraryPlugin : Plugin<Project> { |
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.
remove this, if required create an extension function and call it where it's required like we did configureKotlinMultiplatform() or configureKotlinAndroid
build.gradle.kts
Outdated
| } | ||
| } | ||
|
|
||
| subprojects { |
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.
https://kotlinlang.org/docs/dokka-gradle.html#multi-project-configuration
subprojects {
apply(plugin = "org.jetbrains.dokka")
// configure only the HTML task
tasks.dokkaHtmlPartial {
outputDirectory.set(layout.buildDirectory.dir("docs/partial"))
}
// configure all format tasks at once
tasks.withType<DokkaTaskPartial>().configureEach {
dokkaSourceSets.configureEach {
includes.from("README.md")
}
}
}
feature/accounts/build.gradle.kts
Outdated
| plugins { | ||
| alias(libs.plugins.mifospay.cmp.feature) | ||
| alias(libs.plugins.kotlin.parcelize) | ||
| alias(libs.plugins.dokka) |
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.
remove these plugins alias(libs.plugins.dokka) and alias(libs.plugins.mifospay.feature.library)
|
A lot of solutions exist for any given problem, however, adhering to established architectural principles and guidelines streamlines future management and maintenance. |
…ion and update build configuration
…ion and update build configuration
|
@niyajali fix(shared): replace Replaced Also ran Dokka for core modules to generate up-to-date documentation without issues. |
niyajali
left a comment
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 looks good to me! Once you've implemented the requested changes, please mention us for review—we'll approve and merge it promptly.
Also, moving forward, let's prioritize documenting the core module first. This will help maintain consistency across the project.
build.gradle.kts
Outdated
|
|
||
| // root build.gradle.kts | ||
| tasks.register<DokkaMultiModuleTask>("dokkaHtmlMultiModule") { | ||
| outputDirectory.set(buildDir.resolve("dokka")) |
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.
rename the output dir name is to docs
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.
ok
gradle.properties
Outdated
|
|
||
| # Enable configuration caching between builds. | ||
| org.gradle.configuration-cache=true | ||
| org.gradle.configuration-cache=false |
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.
Let's enable configuration caching to improve build performance. While it may occasionally fail due to cache mismatches, running clean and rebuilding typically resolves the issue. Disabling it would significantly slow down our builds, so the trade-off is worth 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.
ok
gradle/libs.versions.toml
Outdated
| spotless = { id = "com.diffplug.spotless", version.ref = "spotlessVersion" } | ||
| version-catalog-linter = { id = "io.github.pemistahl.version-catalog-linter", version.ref = "versionCatalogLinterVersion" } | ||
|
|
||
| dokka = { id = "org.jetbrains.dokka", version = "2.0.0" } |
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.
use the variable dokkaGradlePlugin for version number like version.ref="dokkaGradlePlugin"
Please don't resolve the conversion even though you fixed it.
gradle/libs.versions.toml
Outdated
| androidx-tracing-ktx = { group = "androidx.tracing", name = "tracing-ktx", version.ref = "androidxTracing" } | ||
|
|
||
| dokka-gradle-plugin = { module = "org.jetbrains.dokka:dokka-gradle-plugin", version.ref = "dokkaGradlePlugin" } | ||
| kotlinx-coroutines-core-v173 = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-core", version.ref = "kotlinxCoroutinesCore" } |
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.
Remove this duplicate library, we do have kotlinx-coroutines-core use that one instead.
mifospay-shared/build.gradle.kts
Outdated
| implementation(projects.feature.qr) | ||
| implementation(projects.feature.merchants) | ||
| implementation(projects.feature.upiSetup) | ||
| implementation(libs.kotlinx.coroutines.core.v173) |
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.
use the existing kotlinx-coroutines-core library
| viewModel.logOut() | ||
| navController.navigate(LOGIN_GRAPH) { | ||
| popUpTo(navController.graph.id) { | ||
| popUpTo(navController.graph.findStartDestination().route ?: "") { |
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 confirm this functionality works as expected after the changes, please provide screen recordings demonstrating:
- login into the account
- Navigation to a sample screen
- Logout flow
- Back button behavior – Verify whether pressing 'Back' after logout reopens the app or stays on the login screen
| // avoid building up a large stack of destinations | ||
| // on the back stack as users select items | ||
| popUpTo(graph.findStartDestination().id) { | ||
| popUpTo(graph.findStartDestination().route ?: "") { |
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.
Similar like above please upload video to verify navigation is working as expected or not
| // avoid building up a large stack of destinations | ||
| // on the back stack as users select items | ||
| popUpTo(navController.graph.findStartDestination().id) { | ||
| popUpTo(navController.graph.findStartDestination().route ?: "") { |
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.
Similar like above please upload video to verify navigation is working as expected or not
| * applying the same configuration to the target Project. | ||
| */ | ||
| // Add this in a shared convention plugin or utilities script | ||
| fun Project.configureFeatureDocumentation() { |
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 can remove these configs since we've already defined similar ones in the root build.gradle.kts, which apply project-wide. These should work as expected. Please let us know if you think these serve a different purpose.
…enabling configuration caching
|
The |
|
@rohitlokhande47 Hi, is there's no update on this PR, have you tested the navigation is it working as before? |
…docs-and-generate-documentation
Pull Request
Issue Fix
Fixes #473
Jira Task: [MPW-629](https://mifospay.atlassian.net/browse/MPW-629)
Screenshots and Video
Screen.Recording.2025-04-07.at.9.39.09.AM.mov
Description
Added documentation comments to the codebase and configured Dokka for API documentation generation.
This PR:
The documentation focuses on explaining the architecture and functionality of components within the accounts feature, making the codebase more maintainable and accessible to new contributors.
No functional code changes were made, only documentation and configuration updates.
AndroidStyle.xmlstyle template to your code in Android Studio../gradlew checkto make sure you didn't break anything[