Skip to content

Conversation

@jmdobry
Copy link
Member

@jmdobry jmdobry commented Aug 19, 2016

Fixes #175

  • - Sinks
  • - Logs
@jmdobry
Copy link
Member Author

jmdobry commented Aug 19, 2016

@ace-n Ready for review, when you have a moment.

@codecov-io
Copy link

codecov-io commented Aug 20, 2016

Current coverage is 87.42% (diff: 97.87%)

Merging #183 into master will increase coverage by 1.10%

@@ master #183 diff @@ ========================================== Files 58 57 -1 Lines 2383 2409 +26 Methods 0 0 Messages 0 0 Branches 0 0 ========================================== + Hits 2057 2106 +49  + Misses 326 303 -23  Partials 0 0 

Powered by Codecov. Last update 47baf19...b5f6fde

config.filter = options.filter;
}
if (options.limit) {
config.pageSize = options.limit;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we rename the options values to the names config uses? That way we can just pass options in as config.

Copy link
Member Author

@jmdobry jmdobry Aug 21, 2016

Choose a reason for hiding this comment

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

Tried that. gcloud-node throws an error if the options contains keys that aren't official options.


'use strict';

var gcloud = require('google-cloud');
Copy link
Contributor

@ace-n ace-n Aug 21, 2016

Choose a reason for hiding this comment

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

😢 (is there any way to not require the whole thing?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but would have to do this instead:

var Logging = require('@google-cloud/logging'); var Storage = require('@google-cloud/storage'); var BigQuery = require('@google-cloud/bigquery'); var PubSub = require('@google-cloud/pubsub');
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, makes sense.

@ace-n
Copy link
Contributor

ace-n commented Aug 21, 2016

LGTM, other than my comments.

@jmdobry
Copy link
Member Author

jmdobry commented Aug 21, 2016

Cool. Just pushed again, will merge when green.

@jmdobry
Copy link
Member Author

jmdobry commented Aug 21, 2016

Thanks!

NimJay pushed a commit that referenced this pull request Nov 18, 2022
NimJay pushed a commit that referenced this pull request Nov 18, 2022
jsimonweb pushed a commit to jsimonweb/nodejs-docs-samples that referenced this pull request Dec 12, 2022
kweinmeister pushed a commit that referenced this pull request Jan 11, 2023
🤖 I have created a release *beep* *boop* --- ## [2.0.0](googleapis/nodejs-retail@v1.8.1...v2.0.0) (2022-06-20) ### ⚠ BREAKING CHANGES * update library to use Node 12 (#181) ### Features * allow users to disable spell check in search requests ([#183](googleapis/nodejs-retail#183)) ([05005ea](googleapis/nodejs-retail@05005ea)) ### Bug Fixes * **deps:** update dependency @google-cloud/bigquery to v6 ([#186](googleapis/nodejs-retail#186)) ([fc07923](googleapis/nodejs-retail@fc07923)) ### Build System * update library to use Node 12 ([#181](googleapis/nodejs-retail#181)) ([809853f](googleapis/nodejs-retail@809853f)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants