Skip to content

[Cleanup] storage/s3-sdk: Adds docstrings, return value instead of print #9948

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 10 commits into from
May 17, 2023

Conversation

askmeegs
Copy link
Contributor

@askmeegs askmeegs commented May 15, 2023

Checklist

@askmeegs askmeegs requested review from a team as code owners May 15, 2023 15:28
@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: storage Issues related to the Cloud Storage API. labels May 15, 2023
@askmeegs askmeegs force-pushed the fixit-mokeefe-cleanup-storage-sdk branch from 7e9bc5d to ef3c246 Compare May 15, 2023 15:31
@askmeegs askmeegs force-pushed the fixit-mokeefe-cleanup-storage-sdk branch from 3832870 to c03bbd8 Compare May 15, 2023 16:03
Copy link
Collaborator

@dandhlee dandhlee left a comment

Choose a reason for hiding this comment

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

Are there any reason to switch to returning a list rather than printing them?

@engelke
Copy link
Contributor

engelke commented May 15, 2023

Are there any reason to switch to returning a list rather than printing them?

Yes. We want to test against returned values rather than captured strings.

Copy link
Contributor

@engelke engelke left a comment

Choose a reason for hiding this comment

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

Please remove "capsys" parameters and fix lint's nitpicking. You don't need to wait for another review from me. LGTM.

@tritone
Copy link
Contributor

tritone commented May 15, 2023

Are there any reason to switch to returning a list rather than printing them?

Yes. We want to test against returned values rather than captured strings.

Have we changed that in general across samples? I see this in the standards still: https://googlecloudplatform.github.io/samples-style-guide/#result

@engelke engelke added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label May 15, 2023
@askmeegs askmeegs force-pushed the fixit-mokeefe-cleanup-storage-sdk branch from 82e3227 to 79a003f Compare May 15, 2023 21:54
@BenWhitehead BenWhitehead removed their assignment May 16, 2023
@engelke
Copy link
Contributor

engelke commented May 17, 2023

Are there any reason to switch to returning a list rather than printing them?

Yes. We want to test against returned values rather than captured strings.

Have we changed that in general across samples? I see this in the standards still: https://googlecloudplatform.github.io/samples-style-guide/#result

The instructions for FixIt don't require removing printing, just no longer basing tests on that instead of on returned values.

@engelke engelke merged commit 68a3d10 into main May 17, 2023
@engelke engelke deleted the fixit-mokeefe-cleanup-storage-sdk branch May 17, 2023 21:30
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. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants