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

datastore: scope Key creation to include NS. fixes #116. #124

Merged
merged 7 commits into from
Aug 22, 2014

Conversation

stephenplusplus
Copy link
Contributor

RE: #116

I actually have 2 different implementations for this, but this was the one I thought was the most logical.

When a user creates a dataset, they can provide a namespace. Then, anytime they need a key, instead of datastore.key(), they call datasetInstance.key() - the key will be created with the namespace already bound to the Key.

The other implementation removed any association of a namespace from a Key object, which felt wrong.

PTAL & LMKWYT.

@rakyll
Copy link
Contributor

rakyll commented Aug 21, 2014

This sounds better. There are some cases datastore.key() would be useful to have such as migrating data from one namespace to the other. I think we may need to support namespaces for createQuery optionally too.

@stephenplusplus
Copy link
Contributor Author

Sounds reasonable, I'll add that in. I also think I may have missed some
doc updates, not sure. Will send through another commit soon.

On Thursday, August 21, 2014, Burcu Dogan notifications@github.com wrote:

This sounds better. There are some cases datastore.key() would be useful
to have such as migrating data from one namespace to the other. I think we
may need to support namespaces for createQuery optionally too.


Reply to this email directly or view it on GitHub
#124 (comment)
.

@stephenplusplus
Copy link
Contributor Author

Made the updates. I didn't add Datastore.key back in, instead allowing for control from the dataset instance level. I think having two places to create one is more confusing than having two ways to configure one. I'll just paste in the documentation to show the changes :)

/**
 * Helper to create a Key object, scoped to the dataset's namespace by default.
 *
 * You may also specify a configuration object to define a namespace and path.
 *
 * @example
 * ```js
 * var key;
 *
 * // Create a key from the dataset's namespace.
 * key = dataset.key('Company', 123);
 *
 * // Create a key from a provided namespace and path.
 * key = dataset.key({
 *   namespace: 'My-NS',
 *   path: ['Company', 123]
 * });
 * ```
 */
Dataset.prototype.key = function(keyConfig) {
  // ...

And I took a similar approach with createQuery:

/**
 * Create a query from the current dataset to query the specified kinds, scoped
 * to the namespace provided at the initialization of the dataset.
 *
 * @borrows {module:datastore/query} as createQuery
 *
 * @param {string=} ns - Optional namespace.
 * @param {string|array} kinds - Kinds to query.
 *
 * @example
 * ```js
 * var query;
 *
 * // If your dataset was scoped to a namespace at initialization, your query
 * // will likewise be scoped to that namespace.
 * query = dataset.createQuery(['Lion', 'Chimp']);
 *
 * // However, you may override the namespace per query.
 * query = dataset.createQuery('AnimalNamespace', ['Lion', 'Chimp']);
 *
 * // You may also remove the namespace altogether.
 * query = dataset.createQuery(null, ['Lion', 'Chimp']);
 * ```
 * @return {module:datastore/query}
 */
Dataset.prototype.createQuery = function(ns, kinds) {
  // ...

@@ -38,7 +42,7 @@ var util = require('../common/util.js');
* ```
*/
function Query(namespace, kinds) {
this.namespace = namespace;
this.namespace = namespace || '';

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

This comment was marked as spam.

@rakyll
Copy link
Contributor

rakyll commented Aug 21, 2014

Added some comments, otherwise LGTM.

].forEach(function(query) {
assert.strictEqual(query.namespace, undefined);
});
});

This comment was marked as spam.

@stephenplusplus
Copy link
Contributor Author

undefined -> null 🍻

@silvolu
Copy link
Contributor

silvolu commented Aug 22, 2014

I see all of @rakyll's comments resolved, merging.

silvolu added a commit that referenced this pull request Aug 22, 2014
datastore: scope Key creation to include NS. fixes #116.
@silvolu silvolu merged commit 3297751 into googleapis:master Aug 22, 2014
sofisl pushed a commit that referenced this pull request Sep 15, 2022
sofisl pushed a commit that referenced this pull request Oct 11, 2022
sofisl pushed a commit that referenced this pull request Nov 11, 2022
See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [jsdoc](https://github.com/jsdoc/jsdoc) | [`^3.6.6` -> `^4.0.0`](https://renovatebot.com/diffs/npm/jsdoc/3.6.11/4.0.0) | [![age](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/compatibility-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/npm/jsdoc/4.0.0/confidence-slim/3.6.11)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>jsdoc/jsdoc</summary>

### [`v4.0.0`](https://github.com/jsdoc/jsdoc/blob/HEAD/CHANGES.md#&#8203;400-November-2022)

[Compare Source](https://github.com/jsdoc/jsdoc/compare/3.6.11...084218523a7d69fec14a852ce680f374f526af28)

-   JSDoc releases now use [semantic versioning](https://semver.org/). If JSDoc makes
    backwards-incompatible changes in the future, the major version will be incremented.
-   JSDoc no longer uses the [`taffydb`](https://taffydb.com/) package. If your JSDoc template or
    plugin uses the `taffydb` package, see the
    [instructions for replacing `taffydb` with `@jsdoc/salty`](https://github.com/jsdoc/jsdoc/tree/main/packages/jsdoc-salty#use-salty-in-a-jsdoc-template).
-   JSDoc now supports Node.js 12.0.0 and later.

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "after 9am and before 3pm" (UTC), Automerge - At any time (no schedule defined).

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

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the rebase/retry checkbox.

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

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

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/nodejs-storage-transfer).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xNy4xIiwidXBkYXRlZEluVmVyIjoiMzQuMTcuMSJ9-->
sofisl pushed a commit that referenced this pull request Nov 11, 2022
This PR was generated using Autosynth. 🌈

Synth log will be available here:
https://source.cloud.google.com/results/invocations/6dcea365-b918-4c52-b1db-d7a62c956000/targets

- [ ] To automatically regenerate this PR, check this box.

Source-Link: googleapis/synthtool@89c849b
Source-Link: googleapis/synthtool@a783321
Source-Link: googleapis/synthtool@b7413d3
Source-Link: googleapis/synthtool@5f6ef0e
sofisl pushed a commit that referenced this pull request Nov 11, 2022
sofisl pushed a commit that referenced this pull request Nov 11, 2022
Fixes googleapis/synthtool#1103
Source-Link: googleapis/synthtool@c3e41da
Post-Processor: gcr.io/repo-automation-bots/owlbot-nodejs:latest@sha256:e37a815333a6f3e14d8532efe90cba8aa0d34210f8c0fdbdd9e6a34dcbe51e96
sofisl added a commit that referenced this pull request Nov 11, 2022
* build!: Update library to use Node 12

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
sofisl pushed a commit that referenced this pull request Nov 11, 2022
Source-Link: googleapis/synthtool@d229a12
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-nodejs:latest@sha256:74ab2b3c71ef27e6d8b69b1d0a0c9d31447777b79ac3cd4be82c265b45f37e5e
sofisl pushed a commit that referenced this pull request Nov 11, 2022
Committer: @summer-ji-eng
PiperOrigin-RevId: 434859890

Source-Link: googleapis/googleapis@bc2432d

Source-Link: googleapis/googleapis-gen@930b673
Copy-Tag: eyJwIjoiLmdpdGh1Yi8uT3dsQm90LnlhbWwiLCJoIjoiOTMwYjY3MzEwM2U5MjUyM2Y4Y2ZlZDM4ZGVjZDdkM2FmYWU4ZWJlNyJ9
sofisl pushed a commit that referenced this pull request Nov 16, 2022
- [ ] Regenerate this pull request now.

PiperOrigin-RevId: 392067151

Source-Link: googleapis/googleapis@06345f7

Source-Link: googleapis/googleapis-gen@95882b3
sofisl pushed a commit that referenced this pull request Nov 17, 2022
sofisl pushed a commit that referenced this pull request Nov 30, 2022
sofisl pushed a commit that referenced this pull request Jan 17, 2023
* fix: synth.py should copy over all versions

* gen: add version v1p3beta1 to synth.py

* fix: run smoke-test

* gen: Vision API v1p3beta1 googleapis/googleapis@78abf01

* style: npm run prettier
sofisl pushed a commit that referenced this pull request Jan 26, 2023
…d invalid_row_count to ImportFeatureValuesResponse and ImportFeatureValuesOperationMetadata (#124)

PiperOrigin-RevId: 372952726
Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Co-authored-by: Benjamin E. Coe <bencoe@google.com>
Release-As: 1.7.0
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

Successfully merging this pull request may close these issues.

3 participants