feat(storage): easier mocks with BucketMetadata
#9886
Conversation
product-auto-label
bot
added
the
api: storage
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
Codecov ReportBase: 94.17% // Head: 94.17% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #9886 +/- ##
=======================================
Coverage 94.17% 94.17%
=======================================
Files 1495 1495
Lines 139977 140051 +74
=======================================
+ Hits 131826 131896 +70
- Misses 8151 8155 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @coryan)
| if (!json.contains("owner")) return Status{}; | ||
| Owner o; | ||
| o.entity = json["owner"].value("entity", ""); | ||
| o.entity_id = json["owner"].value("entityId", ""); |
There was a problem hiding this comment.
Q. Are we at all concerned about repeated lookups?
There was a problem hiding this comment.
I doubt it is in the critical path for anything, but there is no reason to duplicate work either. Fixed.
There was a problem hiding this comment.
Without wanting to derail you any further, it seems like we could still go from 2 lookups to 1 using json.find("owner") (not that I'm familiar with the API).
There was a problem hiding this comment.
Tracking that in #9887
If you mean this one:
I ignore those, they have been around since approximately forever. I am not sure we are doing anything "wrong". My interpretation is that they happen whenever a PR starts is based on an older "HEAD" than the current one (e.g. because something was merged after the PR was created). |
Populating all the fields for `BucketMetadata` was somewhat difficult. The class tried to prevent applications from setting fields that only the server would set. I thought about adding a `BucketMetadataBuilder`, but that would duplicate a lot of code without preventing anything. In addition, all the other libraries use protos that allow any field to be set, and that does not seemt to create a lot of trouble or confusion. I also stopped using the "composition-by-private-inheritance" base because it does not save enough code. I expect this class will go away once I make similar changes to `ObjectMetadata`.
coryan
force-pushed
the
feat-storage-bucket-metadata-builder
branch
from
aa7232f
to
0ef6d38
Compare
|
Google Cloud Build Logs
ℹ️ NOTE: Kokoro logs are linked from "Details" below. |
I was more interested in the other one |
Candidly I don't know. I tend to ignore those and only worry about large-ish changes in coverage. |
Populating all the fields for
BucketMetadatawas somewhat difficult. The class tried to prevent applications from setting fields that only the server would set. I thought about adding aBucketMetadataBuilder, but that would duplicate a lot of code without preventing anything. In addition, all the other libraries use protos that allow any field to be set, and that does not seemt to create a lot of trouble or confusion.I also stopped using the "composition-by-private-inheritance" base because it does not save enough code. I expect this class will go away once I make similar changes to
ObjectMetadata.Part of the work for #8929, I also think it will help with #7142
This change is