feat(google_cloud_storage): implement moveObject support#215
feat(google_cloud_storage): implement moveObject support#215sigurdm wants to merge 3 commits intogoogleapis:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully implements the moveObject functionality, adding both a top-level method in Storage and a convenience method in StorageObject. The implementation correctly constructs the API request URL and handles conditional parameters for idempotency as described in the Google Cloud Storage API documentation. The new test file move_object_test.dart provides comprehensive coverage, including success cases, StorageObject usage, conditional failure, and thorough mock request verification. The code adheres to good Dart practices, is well-documented, and integrates seamlessly with the existing library structure. No specific issues or critical improvement opportunities were identified in the changes.
|
/gcbrun |
|
Hmmm...why didn't /gcbrun trigger the GCB? |
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
Holy crap you did this quicky!
I'm not sure why the GCB tests aren't running. I would have thought that they'd run automatically for you. And I can't seem to manually trigger them.
| /// | ||
| /// If set, [ifGenerationMatch] makes the operation conditional on whether | ||
| /// the destination object's current generation matches the given value. | ||
| /// A value of `0` indicates that the destination object must not already |
There was a problem hiding this comment.
| /// A value of `0` indicates that the destination object must not already | |
| /// A value of `BigInt.zero` indicates that the destination object must not already |
|
|
||
| /// Moves this [Google Cloud Storage object][] to a new [newName]. | ||
| /// | ||
| /// This operation is atomic and idempotent if [ifSourceGenerationMatch] or |
There was a problem hiding this comment.
Is that true? My intuition would be that both have to be set.
| ); | ||
|
|
||
| expect(moved.name, 'dest.txt'); | ||
| expect(moved.bucket, bucketName); |
There was a problem hiding this comment.
Maybe check that the metageneration is 1?
| }); | ||
| }); | ||
|
|
||
| test('mock request verification', () async { |
There was a problem hiding this comment.
I usually use mocks to test retries. Do you think those tests have value? I'm not sure.
|
/gcbrun |
|
Now "/gcbrun" is working - maybe there was some transient infrastructure problem? |
sigurdm
changed the title
2 participants
Fixes #195