Skip to content

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

Merged
merged 11 commits into from
Sep 12, 2022
Merged

Add documentation for DackkaPlugin #4026

merged 11 commits into from
Sep 12, 2022

Conversation

daymxn
Copy link
Member

@daymxn daymxn commented Aug 24, 2022

In accordance with b/240169898, and more specifcally b/240169649- this PR adds a README.md and DackkaPlugin.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.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

buildSrc Test Results

18 tests   18 ✔️  2m 33s ⏱️
  4 suites    0 💤
  4 files      0

Results for commit 7033aa0.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 24, 2022

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Aug 24, 2022

Coverage Report 1

Affected Products

  • firebase-database

    Overall coverage changed from 50.17% (3280a27) to 50.16% (61a7608) by -0.01%.

    FilenameBase (3280a27)Merge (61a7608)Diff
    DefaultPersistenceManager.java74.76%75.73%+0.97%
    DoubleNode.java100.00%88.24%-11.76%
  • firebase-firestore

    Overall coverage changed from 44.32% (3280a27) to 44.42% (61a7608) by +0.10%.

    FilenameBase (3280a27)Merge (61a7608)Diff
    AsyncQueue.java78.11%78.61%+0.50%
    Datastore.java21.55%23.28%+1.72%
    DeleteMutation.java90.48%95.24%+4.76%
    FirestoreChannel.java16.38%18.10%+1.72%
    FirestoreClient.java31.25%36.11%+4.86%
    GrpcCallProvider.java62.35%69.41%+7.06%
    IndexBackfiller.java89.06%93.75%+4.69%
    LruGarbageCollector.java90.65%93.46%+2.80%
    SetMutation.java97.22%94.44%-2.78%
    SQLitePersistence.java86.34%86.89%+0.55%
  • firebase-storage

    Overall coverage changed from ? (3280a27) to 86.33% (61a7608) by ?.

    46 individual files with coverage change

    FilenameBase (3280a27)Merge (61a7608)Diff
    ActivityLifecycleListener.java?74.14%?
    AdaptiveStreamBuffer.java?84.62%?
    CancelException.java?100.00%?
    CancellableTask.java?100.00%?
    ControllableTask.java?100.00%?
    DeleteNetworkRequest.java?100.00%?
    DeleteStorageTask.java?100.00%?
    ExponentialBackoffSender.java?86.00%?
    FileDownloadTask.java?80.00%?
    FirebaseStorage.java?85.11%?
    FirebaseStorageComponent.java?100.00%?
    GetDownloadUrlTask.java?96.77%?
    GetMetadataNetworkRequest.java?100.00%?
    GetMetadataTask.java?85.19%?
    GetNetworkRequest.java?100.00%?
    HttpURLConnectionFactory.java?0.00%?
    HttpURLConnectionFactoryImpl.java?50.00%?
    ListNetworkRequest.java?100.00%?
    ListResult.java?100.00%?
    ListTask.java?85.71%?
    NetworkRequest.java?86.67%?
    OnPausedListener.java?0.00%?
    OnProgressListener.java?0.00%?
    ResumableNetworkRequest.java?100.00%?
    ResumableUploadByteRequest.java?90.91%?
    ResumableUploadCancelRequest.java?100.00%?
    ResumableUploadQueryRequest.java?100.00%?
    ResumableUploadStartRequest.java?95.24%?
    Slashes.java?88.24%?
    Sleeper.java?0.00%?
    SleeperImpl.java?33.33%?
    SmartHandler.java?87.50%?
    StorageException.java?69.09%?
    StorageMetadata.java?86.34%?
    StorageReference.java?89.94%?
    StorageReferenceUri.java?100.00%?
    StorageRegistrar.java?100.00%?
    StorageTask.java?84.89%?
    StorageTaskManager.java?100.00%?
    StorageTaskScheduler.java?100.00%?
    StreamDownloadTask.java?88.89%?
    TaskListenerImpl.java?100.00%?
    UpdateMetadataNetworkRequest.java?100.00%?
    UpdateMetadataTask.java?82.14%?
    UploadTask.java?83.09%?
    Util.java?73.24%?

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/UnsDCkaS1F.html

@github-actions
Copy link
Contributor

github-actions bot commented Aug 24, 2022

Unit Test Results

   394 files  +   389     394 suites  +389   16m 41s ⏱️ + 16m 34s
4 717 tests +4 706  4 695 ✔️ +4 684  22 💤 +22  0 ±0 
4 733 runs  +4 722  4 711 ✔️ +4 700  22 💤 +22  0 ±0 

Results for commit 7033aa0. ± Comparison against base commit d84113e.

♻️ This comment has been updated with latest results.

Copy link
Member

@vkryachko vkryachko left a 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?

@daymxn
Copy link
Member Author

daymxn commented Aug 25, 2022

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?

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?

@vkryachko
Copy link
Member

I think having it live in the kdoc would eat up a lot of lines for something you (typically) expect to be brief

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 java.util.List as one example.

That said I don't feel strongly, but slightly prefer having this documentation inline with the class.

@daymxn
Copy link
Member Author

daymxn commented Aug 25, 2022

I think having it live in the kdoc would eat up a lot of lines for something you (typically) expect to be brief

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 java.util.List as one example.

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?

@vkryachko
Copy link
Member

Because reading in the IDE is not the most pleasant

Depends on the IDE I guess, but in Android Studio/IntelliJ it renders nicely.

Screen Shot 2022-08-26 at 9 38 42 AM
y

@daymxn daymxn requested a review from vkryachko September 8, 2022 19:02
@daymxn
Copy link
Member Author

daymxn commented Sep 9, 2022

/retest

@daymxn daymxn merged commit 5892717 into master Sep 12, 2022
@daymxn daymxn deleted the daymon-improve-docs-dackka branch September 12, 2022 21:22
qdpham13 pushed a commit that referenced this pull request Sep 13, 2022
* 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
@firebase firebase locked and limited conversation to collaborators Oct 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants