feat(storage-ktx): destructure ListResult and TaskSnapshots #1644

thatfiredev · 2020-06-11T20:35:10Z

This is actually a Feature Request. It should add Kotlin destructuring declarations to the classes: FileDownloadTask.TaskSnapshot, StreamDownloadTask.TaskSnapshot, UploadTask.TaskSnapshot and ListResult.

With the destructuring declarations, instead of:

fileDownloadTask.addOnSuccessListener { taskSnapshot ->
    val bytesTransferred = taskSnapshot.bytesTransferred
    val totalCount = taskSnapshot.totalByteCount
    val progress = bytesTransferred * 100 / totalCount
}

Kotlin developers will be able to use:

fileDownloadTask.addOnSuccessListener { (bytesTransferred, totalCount) ->
    val progress = bytesTransferred * 100 / totalCount
}

Here's a full list of all the declarations added in this PR:

Class	Destructured
FileDownloadTask.TaskSnapshot	(bytesTransferred, totalByteCount)
StreamDownloadTask.TaskSnapshot	(bytesTransferred, totalByteCount, stream)
UploadTask.TaskSnapshot	(bytesTransferred, totalByteCount, metadata, uploadSessionUri)
ListResult	(items, prefixes, pageToken)

I should also point out that these arguments are optional, so for example, if a developer wishes to only get ListResult.items and not ListResult.prefixes or ListResult.pageToken, he may omit them:

storageRef.listAll().addOnSuccessListener { (items) ->
    // handle results
}

google-oss-bot · 2020-06-11T20:35:25Z

Hi @rosariopfernandes. Thanks for your PR.

I'm waiting for a firebase member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rlazo · 2020-07-06T11:01:43Z

Hi @rosariopfernandes thanks for the proposal. I like the idea of adding destructure here! We'll evaluate this through our internal API review process. I'll keep you posted.

firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

…-sdk into rpf-storage-ktx-desctructure � Conflicts: � firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

…to rpf-storage-ktx-desctructure # Conflicts: # firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

vkryachko · 2020-07-13T17:21:02Z

/ok-to-test

google-oss-bot · 2020-07-13T17:27:05Z

Coverage Report

Affected SDKs

`firebase-storage`

SDK overall coverage changed from ? (a2dae12) to 85.44% (02fdf45d) by ?.

Click to show coverage changes in 45 files.

Filename	Base (`a2dae12`)	Head (02fdf45d)	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.05%	?
FileDownloadTask.java	?	77.60%	?
FirebaseStorage.java	?	84.34%	?
FirebaseStorageComponent.java	?	100.00%	?
GetDownloadUrlTask.java	?	96.67%	?
GetMetadataNetworkRequest.java	?	100.00%	?
GetMetadataTask.java	?	84.62%	?
GetNetworkRequest.java	?	100.00%	?
HttpURLConnectionFactory.java	?	0.00%	?
HttpURLConnectionFactoryImpl.java	?	50.00%	?
ListNetworkRequest.java	?	100.00%	?
ListResult.java	?	100.00%	?
ListTask.java	?	85.19%	?
NetworkRequest.java	?	89.29%	?
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.00%	?
Slashes.java	?	82.35%	?
Sleeper.java	?	0.00%	?
SleeperImpl.java	?	33.33%	?
SmartHandler.java	?	87.50%	?
StorageException.java	?	65.45%	?
StorageMetadata.java	?	86.34%	?
StorageReference.java	?	89.88%	?
StorageRegistrar.java	?	100.00%	?
StorageTask.java	?	84.29%	?
StorageTaskManager.java	?	100.00%	?
StorageTaskScheduler.java	?	100.00%	?
StreamDownloadTask.java	?	88.89%	?
TaskListenerImpl.java	?	100.00%	?
UpdateMetadataNetworkRequest.java	?	100.00%	?
UpdateMetadataTask.java	?	80.00%	?
UploadTask.java	?	79.85%	?
Util.java	?	65.57%	?

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (02fdf45d) is created by Prow via merging commits: a2dae12 6ead390.

google-oss-bot · 2020-07-13T17:28:54Z

Binary Size Report

Affected SDKs

firebase-storage-ktx

Type Base (a2dae12) Head (02fdf45d) Diff

aar 6.14 kB 7.00 kB +863 B (+14.1%)

apk (release) 1.43 MB 1.43 MB +280 B (+0.0%)

Test Logs

Notes

Head commit (02fdf45d) is created by Prow via merging commits: a2dae12 6ead390.

…m/rosariopfernandes/firebase-android-sdk into rpf-storage-ktx-desctructure � Conflicts: � firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt

schmidt-sebastian · 2020-07-15T17:13:02Z

@rosariopfernandes Just wanted to chime in since I am cc'ed on this: This PR will likely sit here for a while (2-3 weeks) as we obtain internal API approval. There is nothing more we need.

thatfiredev · 2020-07-15T17:18:36Z

@schmidt-sebastian Yes, I understand. I was just trying to fix the coverage report. Sorry for flooding your notifications inbox.

rlazo

Hi @rosariopfernandes thanks for your patience, your proposal has been approved 👍 Just address the comments below and we can merge your change. Thanks for your help!

firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt

rlazo · 2020-08-10T16:14:03Z

@rosariopfernandes check-changed is failing with

> Task :firebase-storage:ktx:compileReleaseUnitTestKotlin FAILED
w: Argument -module-name is passed multiple times. Only the last value will be used: com.google.firebase-firebase-storage-ktx
e: /home/prow/go/src/github.com/firebase/firebase-android-sdk/firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt: (29, 36): Unresolved reference: TestUtil
e: /home/prow/go/src/github.com/firebase/firebase-android-sdk/firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt: (119, 30): Unresolved reference: TestUtil
e: /home/prow/go/src/github.com/firebase/firebase-android-sdk/firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt: (119, 50): Type inference failed: Not enough information to infer parameter T in inline fun <T> listOf(): List<T>
Please specify it explicitly.

e: /home/prow/go/src/github.com/firebase/firebase-android-sdk/firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt: (119, 60): Type inference failed: Not enough information to infer parameter T in inline fun <T> listOf(): List<T>
Please specify it explicitly.

thatfiredev · 2020-08-10T18:09:33Z

@rlazo Because the TestUtil class was on a different module's test directory, it was inaccessible from the -ktx module. I tried adding it to the -ktx sourceSet like this:

sourceSets {
        // ...
        test.java {
            // ...
            srcDir '../src/test/java'
        }
        // ...
}

But it would try to import all the other test classes as well, and some of them were showing errors due to the difference in package names.

As a workaround, I created a new directory (/test/kotlin/) and a new TestUtil class (still under the com.firebase.storage package) exclusively for the ktx module. Seems like it does the trick.

rlazo · 2020-08-10T19:24:23Z

Would it be possible to add it to firebase-storage/src/testUtil/java/com/google/firebase/storage ? That could eliminate the need for a new directory.

thatfiredev · 2020-08-10T21:30:02Z

@rlazo I did that, but needed to make 2 additional changes:

Renamed the class to KtxTestUtil, because it was conflicting with the existing TestUtil class.
Added Java 8 to the ktx module, because some classes from the testUtil package use Java 8 features (e.g. Lambda functions).

google-oss-bot · 2020-08-10T23:20:05Z

@rosariopfernandes: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
smoke-tests	`6ead390`	link	`/test smoke-tests`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

feat(storage-ktx): destructure ListResult and TaskSnapshots

2259a35

googlebot added the cla: yes Override cla label Jun 11, 2020

google-oss-bot added the needs-ok-to-test label Jun 11, 2020

google-oss-bot added the size/M label Jun 11, 2020

test(storage-ktx): add unit tests for destructured classes

e39ca86

rlazo self-requested a review July 6, 2020 11:01

vkryachko reviewed Jul 13, 2020

View reviewed changes

firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt Outdated Show resolved Hide resolved

firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt Outdated Show resolved Hide resolved

MutableList --> List

e3db267

google-oss-bot added size/L and removed size/M labels Jul 13, 2020

Merge branch 'master' of https://github.com/firebase/firebase-android…

8e2e554

…-sdk into rpf-storage-ktx-desctructure � Conflicts: � firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

google-oss-bot added size/S and removed size/L labels Jul 13, 2020

thatfiredev added 2 commits July 13, 2020 18:45

Merge branch 'master' of https://github.com/firebase/firebase-android…

6676660

…-sdk into rpf-storage-ktx-desctructure � Conflicts: � firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

Merge remote-tracking branch 'origin/rpf-storage-ktx-desctructure' in…

6a9ead6

…to rpf-storage-ktx-desctructure # Conflicts: # firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt

google-oss-bot added size/L and removed size/S labels Jul 13, 2020

remove duplicated imports

5dec4e4

google-oss-bot added ok-to-test and removed needs-ok-to-test labels Jul 13, 2020

thatfiredev added 4 commits July 13, 2020 19:36

style: re-organize imports

b08685b

Merge branch 'rpf-storage-ktx-destructure-tests' of https://github.co…

2ac8d82

…m/rosariopfernandes/firebase-android-sdk into rpf-storage-ktx-desctructure � Conflicts: � firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt

test(storage-ktx): add tests for destructuring declarations

d031aae

style: remove wildcard import

aae2a90

rlazo reviewed Aug 10, 2020

View reviewed changes

firebase-storage/ktx/src/main/kotlin/com/google/firebase/storage/ktx/Storage.kt Outdated Show resolved Hide resolved

firebase-storage/ktx/src/test/kotlin/com/google/firebase/storage/ktx/StorageTest.kt Outdated Show resolved Hide resolved

thatfiredev added 2 commits August 10, 2020 17:58

test: create TestUtl.listResult() for ktx tests

3cc4778

chore: add dots to kdocs comments

ff2b0b9

rlazo approved these changes Aug 10, 2020

View reviewed changes

thatfiredev added 4 commits August 10, 2020 18:14

style: explicitly set listOf() type

47f485e

refactor: move TestUtil.listResult to separate package

eaa905f

chore: add copyright to /src/test/kotlin/TestUtil.java

e1d6aef

refactor: google java format

294de57

thatfiredev added 4 commits August 10, 2020 21:31

refactor: move TestUtil from /test/kotlin to /testUtil/java

919bba5

style: google java format

77b4809

TestUtil --> KtxTestUtil

ee5e5eb

chore(firebase-storage-ktx): add java 8

6ead390

thatfiredev requested a review from rlazo August 12, 2020 12:28

rlazo approved these changes Aug 17, 2020

View reviewed changes

rlazo merged commit 8c187fd into firebase:master Aug 17, 2020

thatfiredev deleted the rpf-storage-ktx-desctructure branch August 17, 2020 20:08

thatfiredev mentioned this pull request Aug 20, 2020

docs(storage-ktx): document kotlin destructuring declarations #1900

Closed

firebase locked and limited conversation to collaborators Sep 17, 2020

feat(storage-ktx): destructure ListResult and TaskSnapshots #1644

feat(storage-ktx): destructure ListResult and TaskSnapshots #1644

Uh oh!

Conversation

thatfiredev commented Jun 11, 2020

Uh oh!

google-oss-bot commented Jun 11, 2020

Uh oh!

rlazo commented Jul 6, 2020

Uh oh!

Uh oh!

Uh oh!

vkryachko commented Jul 13, 2020

Uh oh!

google-oss-bot commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-storage

Test Logs

Notes

Uh oh!

google-oss-bot commented Jul 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-storage-ktx

Test Logs

Notes

Uh oh!

schmidt-sebastian commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thatfiredev commented Jul 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rlazo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rlazo commented Aug 10, 2020

Uh oh!

thatfiredev commented Aug 10, 2020

Uh oh!

rlazo commented Aug 10, 2020

Uh oh!

thatfiredev commented Aug 10, 2020

Uh oh!

google-oss-bot commented Aug 10, 2020

Uh oh!

Uh oh!

google-oss-bot commented Jul 13, 2020 •

edited

Loading

`firebase-storage`

google-oss-bot commented Jul 13, 2020 •

edited

Loading

`firebase-storage-ktx`

schmidt-sebastian commented Jul 15, 2020 •

edited

Loading

thatfiredev commented Jul 15, 2020 •

edited

Loading