Skip to content
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

Propose new sample format. #1232

Merged
merged 14 commits into from
Feb 25, 2019
Merged

Propose new sample format. #1232

merged 14 commits into from
Feb 25, 2019

Conversation

kurtisvg
Copy link
Contributor

Updated InspectString and InspectFile snippets to the newly proposed format.

@kurtisvg kurtisvg added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 15, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 15, 2018
@kurtisvg kurtisvg force-pushed the example-sample branch 2 times, most recently from 8ba8b0b to f53ac56 Compare October 15, 2018 17:12
@kurtisvg kurtisvg mentioned this pull request Oct 16, 2018
.build();

// Run request and parse response
InspectContentResponse response = dlp.inspectContent(request);
Copy link
Contributor

Choose a reason for hiding this comment

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

How long does this take? Should I make it async? Do it in it's own thread, or in a separate microservice? How does this code guide my architecture choices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The snippets with longer running operations use PubSub actions for longer running jobs. Do you think that with a comparison between different snippets there is enough suggestions for architecture choices? If not, do you have suggestions on how to better convey it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be inclined to mention it in the preamble (class or method comments).

Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

I like this, I just think there should be some additional context provided to our users. Explaining what situations you might call this vs. some other possible situations that might require something bigger. (ie. architecture hints)

@kurtisvg kurtisvg added the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 22, 2018
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Oct 22, 2018
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

I like the new structure. Much clearer without a CLI.

dlp/src/main/java/dlp/snippets/InspectFile.java Outdated Show resolved Hide resolved
Copy link
Contributor

@tswast tswast left a comment

Choose a reason for hiding this comment

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

LGTM

dlp/src/main/java/dlp/snippets/InspectFile.java Outdated Show resolved Hide resolved
@kurtisvg kurtisvg requested a review from a team February 15, 2019 16:22
@kurtisvg
Copy link
Contributor Author

@dzlier-gcp Ready for another review.

@kurtisvg
Copy link
Contributor Author

@dzlier-gcp ping

@kurtisvg kurtisvg removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 25, 2019
@kurtisvg kurtisvg changed the title [DO NOT MERGE] Propose new sample format. Propose new sample format. Feb 25, 2019
@@ -16,6 +16,7 @@

package dlp.snippets;

// [START dlp_redact_image]
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentional? If it is there's no end tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not intentional. Fixed.

@kurtisvg kurtisvg merged commit 6bb16d1 into master Feb 25, 2019
@kurtisvg kurtisvg deleted the example-sample branch February 25, 2019 22:27
pheonixblade9 pushed a commit to pheonixblade9/java-docs-samples that referenced this pull request Dec 13, 2019
pheonixblade9 pushed a commit to pheonixblade9/java-docs-samples that referenced this pull request Dec 13, 2019
Shabirmean pushed a commit that referenced this pull request Nov 17, 2022
…1.1 (#1232)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.1.0` -> `26.1.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/compatibility-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/confidence-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-automl).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xODQuMiIsInVwZGF0ZWRJblZlciI6IjMyLjE4NC4yIn0=-->
Shabirmean pushed a commit that referenced this pull request Nov 18, 2022
…1.1 (#1232)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.1.0` -> `26.1.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/compatibility-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/confidence-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-automl).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xODQuMiIsInVwZGF0ZWRJblZlciI6IjMyLjE4NC4yIn0=-->
anguillanneuf pushed a commit that referenced this pull request Dec 5, 2022
…1.1 (#1232)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.cloud:libraries-bom](https://cloud.google.com/java/docs/bom) ([source](https://togithub.com/googleapis/java-cloud-bom)) | `26.1.0` -> `26.1.1` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/compatibility-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:libraries-bom/26.1.1/confidence-slim/26.1.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. ⚠ **Warning**: custom changes will be lost.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-automl).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xODQuMiIsInVwZGF0ZWRJblZlciI6IjMyLjE4NC4yIn0=-->
Sita04 pushed a commit that referenced this pull request Feb 7, 2023
* Update DLP snippet with proposal for new sample format.

* Fix InspectFile region tag.

* Fixed package structure and InspectString test.

* Reword comments.

* Fix unintentional toString.

* Seperate API call from response processing.

* Seperate ByteType into two different samples.

* Remove previous tests.

* Updated Redact sample to new format.

* Clean up comments.

* Bump license year.

* Fixed redact test.

* Remove old redact snippet and move region tags.

* Remove region tag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants