Skip to content

Conversation

@IlyaFaer
Copy link

Fixes #10107

@IlyaFaer IlyaFaer added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: storage Issues related to the Cloud Storage API. labels Jan 13, 2020
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 13, 2020
@IlyaFaer
Copy link
Author

IlyaFaer commented Jan 13, 2020

Test_Blob.test_get_iam_policy failed - not related to this PR

"""
value = {k: str(v) for k, v in value.items()}
if value is not None:
value = {k: str(v) for k, v in value.items()}
Copy link
Author

@IlyaFaer IlyaFaer Jan 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

value has tag Optional, so it really looks like a regression
Do we need to do the same thing for bucket labels? I don't see Optional tag for them. Still, stringifying was added for them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the documentation is incorrect. The code is correct in that the property metadata is optional but should not be optional when setting this value.

Copy link
Author

@IlyaFaer IlyaFaer Jan 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've deleted Optional tag. As I see, it was added by some kind of a script

@IlyaFaer IlyaFaer requested review from crwilcox and frankyn January 13, 2020 14:33
@IlyaFaer IlyaFaer marked this pull request as ready for review January 13, 2020 14:34
@IlyaFaer IlyaFaer requested a review from tseaver as a code owner January 13, 2020 14:34
"""
value = {k: str(v) for k, v in value.items()}
if value is not None:
value = {k: str(v) for k, v in value.items()}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the documentation is incorrect. The code is correct in that the property metadata is optional but should not be optional when setting this value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants