Skip to content

adds option to overwrite soap attributes key #73

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

danielsan
Copy link

The FeedService asks for a key named attributes, which conflicts with the current code.

Without breaking any compatibility with existing code we added an extra option to set the name of the attributes key by forwarding it to node-soap.

Copy link

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Mostly:

  1. Update places where we have versions to use DEFAULT_ADWORDS_VERSION so that the code stays the same across version changes
  2. Commented out code should be removed.

readme.md Outdated
@@ -39,7 +39,7 @@ const AdwordsUser = require('node-adwords').AdwordsUser;
const AdwordsConstants = require('node-adwords').AdwordsConstants;

let user = new AdwordsUser({...});
let campaignService = user.getService('CampaignService', 'v201802')
let campaignService = user.getService('CampaignService', 'v201806')

Choose a reason for hiding this comment

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

Why don't we get the version from the constants set instead? DEFAULT_ADWORDS_VERSION / So that the README stays the same for the most part.

readme.md Outdated
@@ -64,7 +64,7 @@ regular api.
const AdwordsReport = require('node-adwords').AdwordsReport;

let report = new AdwordsReport({/** same config as AdwordsUser above */});
report.getReport('v201802', {
report.getReport('v201806', {

Choose a reason for hiding this comment

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

Same here, get version from DEFAULT_ADWORDS_VERSION

readme.md Outdated
@@ -83,7 +83,7 @@ report.getReport('v201802', {
You can also pass in additional headers in case you need to remove the header rows

```js
report.getReport('v201802', {
report.getReport('v201806', {

Choose a reason for hiding this comment

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

and here too.

readme.md Outdated
@@ -120,7 +120,7 @@ You can also use AWQL with Performance Reports

```js
let report = new AdwordsReport({/** same config as AdwordsUser above */});
report.getReport('v201802', {
report.getReport('v201806', {

Choose a reason for hiding this comment

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

and here.

@@ -18,7 +18,7 @@ if (developerToken && refreshToken && clientId && clientSecret && clientCustomer
client_id: clientId,
client_secret: clientSecret,
refresh_token: refreshToken,
version: 'v201802'
version: 'v201806'

Choose a reason for hiding this comment

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

Same here, get from DEFAULT_ADWORDS_VERSION

@danielsan
Copy link
Author

I have removed the comments but the static versions are not actually part of the code.
Do we really want to recommend the users to always use the default version constant?

unoriginalbanter and others added 4 commits December 21, 2018 11:40
* Updated package dependencies

* Added error handling for cases involving requests that do not conform to endpoint WSDL.
Bumps [axios](https://github.com/axios/axios) from 0.18.0 to 0.18.1.
- [Release notes](https://github.com/axios/axios/releases)
- [Changelog](https://github.com/axios/axios/blob/v0.18.1/CHANGELOG.md)
- [Commits](axios/axios@v0.18.0...v0.18.1)

---
updated-dependencies:
- dependency-name: axios
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [lodash](https://github.com/lodash/lodash) from 4.17.10 to 4.17.21.
- [Release notes](https://github.com/lodash/lodash/releases)
- [Commits](lodash/lodash@4.17.10...4.17.21)

---
updated-dependencies:
- dependency-name: lodash
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <support@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

Successfully merging this pull request may close these issues.

3 participants