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

Remove examples if-then-else pattern #618

Closed
mziccard opened this issue Feb 4, 2016 · 6 comments
Closed

Remove examples if-then-else pattern #618

mziccard opened this issue Feb 4, 2016 · 6 comments
Assignees

Comments

@mziccard
Copy link
Contributor

mziccard commented Feb 4, 2016

In bigquery example we have the following patttern:

Table table = getTable(...);
if (table == null) {
  // create table
} else {
  // load data
}

in storage we have:

Blob blob = getBlob(...);
if (blob == null) {
  // create blob
} else {
  // get and update blob's content
}

In both cases we should remove the else branch and rather execute it after table/blob creation.

Also in datastore we do:

Entity entity = getEntity(...);
if (entity = null) {
  // create entity
} else {
  // update entity access type
}

@ajkannan @aozarov @mderka Do you guys think we should remove the else branch also for datastore's example? It makes slightly less sense to me.

@ajkannan
Copy link

ajkannan commented Feb 4, 2016

I agree that the datastore "else" makes a more sense because users typically won't create an entity and right away modify it.

@aozarov
Copy link
Contributor

aozarov commented Feb 4, 2016

My first reaction was that we should not change the datastore example as it is different from the big query example in that the create (the then part of the if~else) also populate the content.

However, this is also true for storage as both parts of the if~else also set the content.
Indeed, the datastore example makes more sense to me as the "else" part
is showing a way to update an entity that already exists by splitting name into
first and last name (as oppose to a complete override of the content as we do for storage)
but maybe that is too much to show for such example (not to mention that typically
this should be done transactionally which would even further complicate the example).

I do think we should modify the big query example, as suggested earlier, to remove the else
so that in the same request we create the table if needed and upload data to it.

Honestly, I think for storage best would be to split the example into 2 separate example
snippets. One for get and display (meta data and content) and one for create.

We could probably do the same for datastore or maybe add some comments to the existing
example and even big query can add a snippet for reading data.

Also, while at it, should resource manager example modified as well? replace just after create is not a typical usage pattern.

Thoughts?

@ajkannan
Copy link

ajkannan commented Feb 4, 2016

I agree with the idea to split "create" and "update" actions in the snippets for storage, datastore, and resource manager.

This is probably slightly more convenient for users as well. If they run a snippet with create/update combined and creation works, but adding/updating data errors, a user can't simply run the same snippet over again. They'll have to modify their code because they can't create the same exact resource again.

@mderka
Copy link

mderka commented Feb 4, 2016

I agree with both @ajkannan and @aozarov, i.e., splitting storage and data store into snippets, and modifying big query to create-if-needed-and-load meaning. All of them make sense. Same for resource manager---short snippets would be my preferred way.

@mziccard
Copy link
Contributor Author

mziccard commented Feb 5, 2016

I agree that we should split create and update, even though I wouldn't expect users to simply copy-paste snippets but rather use them to write their own first simple program.

I will take care of fixing the snippet situation as soon as #614 is merged in. I'll also try to fix #607.

@mziccard mziccard self-assigned this Feb 5, 2016
@aozarov
Copy link
Contributor

aozarov commented Feb 10, 2016

Fixed by #635

@aozarov aozarov closed this as completed Feb 10, 2016
github-actions bot pushed a commit to suztomo/google-cloud-java that referenced this issue Jun 29, 2022
🤖 I have created a release \*beep\* \*boop\*
---
### Updating meta-information for bleeding-edge SNAPSHOT release.
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
github-actions bot pushed a commit that referenced this issue Jul 6, 2022
…ng-dashboard to v2.5.0 (#618)

[![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:google-cloud-monitoring-dashboard](https://togithub.com/googleapis/java-monitoring-dashboards) | `2.4.0` -> `2.5.0` | [![age](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-monitoring-dashboard/2.5.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-monitoring-dashboard/2.5.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-monitoring-dashboard/2.5.0/compatibility-slim/2.4.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.cloud:google-cloud-monitoring-dashboard/2.5.0/confidence-slim/2.4.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 these updates 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-monitoring-dashboards).
suztomo pushed a commit that referenced this issue Feb 1, 2023
* chore: update github actions

* chore: update
Source-Link: googleapis/synthtool@1622741
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:e3746f84955528d0fe24bf2e4df89875d6ce5a036af01b9c326d61a38838523a

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants