-
Notifications
You must be signed in to change notification settings - Fork 625
Add documentation for DackkaPlugin #4026
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
Conversation
Size Report 1Affected ProductsNo changes between base commit (3280a27) and merge commit (61a7608).Test Logs |
Coverage Report 1Affected Products
Test Logs |
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.
Overall looks good, thanks for thoroughly documenting it. Left a few comments below.
A meta point: I wonder if it makes sense to move this documentation closer to the source of truth, namely to the kdoc of the DackkaPlugin. That will make it more easier to find and more likely that this documentation will stay up to date. wdyt?
buildSrc/src/main/java/com/google/firebase/gradle/plugins/DackkaPlugin.md
Outdated
Show resolved
Hide resolved
buildSrc/src/main/java/com/google/firebase/gradle/plugins/DackkaPlugin.md
Outdated
Show resolved
Hide resolved
Hmmm that's a good point. Although the KDOC does already provide a moderate amount of explanation. This doc is supposed to be a bit more in-depth and longer. I think having it live in the kdoc would eat up a lot of lines for something you (typically) expect to be brief. Furthermore, the markdown aspect of it wouldn't be as readable as it is currently. I do think we should move it someplace else though. Currently, it lives right next to the source- which is fine for now. But should we document more of the plugins, it could get out of control fairly quickly. fwiw, we could also add a note to the kdoc about this file? So if folks miss it, they know where to look. Thoughts? |
I don't quite agree with that, I think it's usually brief since there is not much to say about it, but when there is it's ok and even good to make it detailed. i.e. look at That said I don't feel strongly, but slightly prefer having this documentation inline with the class. |
Hmm, but isn't that because they generate javadocs for it? Because reading in the IDE is not the most pleasant. Or would you rather we generate javadocs for documentation instead of providing markdown files? |
/retest |
buildSrc/src/main/java/com/google/firebase/gradle/plugins/DackkaPlugin.kt
Outdated
Show resolved
Hide resolved
* Add Dackka documentation * Added PNG variant of flow diagram * Removed SVG and improved wording * Fixed some grammar mistakes * Added TOC for buildSrc * Fixed some grammar * Fixed explanation about doclava. * Moved docs to DackkaPlugin, and fixed incorrect bug link * Fixed readme pointing to old markdown file * Changed bug link to reflect fixed bug
In accordance with b/240169898, and more specifcally b/240169649- this PR adds a
README.md
andDackkaPlugin.md
to buildSrc.README.md
just serves as a table of contents currently, and should be updated down the line once enough documentation has been created to justify otherwise.DackkaPlugin.md
walks through how Dackka and the DackkaPlugin work, as to help those onboarding understand it.In the future, I'd like to add additional documentation in the repo itself.
Note: I'm not sure if storing the documentation next to the plugin is the best idea. Maybe in something like
docs/plugins
at the root? I'm not completely sure.