nodejs-datastore
nodejs-datastore copied to clipboard
feat: allow setting cursor to null
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
- [ ] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
- [X] Ensure the tests and linter pass (see note below)
- [X] Code coverage does not decrease (if any source code was changed)
Rationale for this change:
The cursor could be returned as null.
It is nice to be able to pass the value directly to the setters.
Note: some tests fails with
3) import/export entities
"before all" hook for "should export entities":
Error: 7 PERMISSION_DENIED: Service account does not have access to Google Cloud Storage file: /nodejs-datastore-system-tests. See https://cloud.google.com/datastore/docs/export-import-entities#permissions for a list of permissions needed. Error details: [email protected] does not have storage.buckets.get access to the Google Cloud Storage bucket. Permission 'storage.buckets.get' denied on resource (or it may not exist).
It is not related to the change in this PR.
Can this be merged?
This change is really similar to https://github.com/googleapis/nodejs-datastore/pull/1067/files. I wonder if we should add tests requiring the new change as well as a test for what proto this new change produces if null is provided.
Note that null is the existing default value (see this.startVal = null; and this.endVal = null;).
So I guess this is already tested.
The only thing this CL makes possible is to call start(null) and end(null) which was not possible before.
Without this CL the client code should be:
if (cursor != null) {
query.start(cursor);
}
With this CL you can do
query.start(cursor);
When cursor is null you will end up with this.startVal == null in both cases:
In the first case because it is the default value.
In the second case because it is explicitly set.
Alright. I was wondering if we should have a test that shows usage that would break the typescript compiler if we didn't have the change, but maybe that's overkill. Seems fine to merge.