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

pass only one error if error happens, fix #100 #107

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@iamstarkov

iamstarkov commented Aug 1, 2015

as titled and checked with my get-tweets package

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Aug 3, 2015

@desmondmorris can I ask you about code-review for this patch?

iamstarkov commented Aug 3, 2015

@desmondmorris can I ask you about code-review for this patch?

@desmondmorris

This comment has been minimized.

Show comment
Hide comment
@desmondmorris

desmondmorris Aug 3, 2015

Owner

Thanks @iamstarkov. I do still think, we may want to defer error parsing to client as per that document, it does not actually say that there can only be a single error delivered in that array, it just shows an example where it does. There is also language there that implies that the error response could be unstructured:

Error responses are served with a non-200-series HTTP code. Usually a JSON response will be attached, but some errors will respond with different kinds of body. In these circumstances where a response struture cannot be parsed, consider the HTTP code’s core meaning to take precedence. For instance, you may occaisionally see a HTTP 404 along with a HTML response. In this case, it’s safe to assume that the content cannot be found (HTTP 404 means “Not Found”).
https://dev.twitter.com/ads/basics/response-codes#Error+Response+Structure

I am just trying to be cautious here as this is a breaking change.

@Roach any thoughts/insight here?
Original issue: #100

Owner

desmondmorris commented Aug 3, 2015

Thanks @iamstarkov. I do still think, we may want to defer error parsing to client as per that document, it does not actually say that there can only be a single error delivered in that array, it just shows an example where it does. There is also language there that implies that the error response could be unstructured:

Error responses are served with a non-200-series HTTP code. Usually a JSON response will be attached, but some errors will respond with different kinds of body. In these circumstances where a response struture cannot be parsed, consider the HTTP code’s core meaning to take precedence. For instance, you may occaisionally see a HTTP 404 along with a HTML response. In this case, it’s safe to assume that the content cannot be found (HTTP 404 means “Not Found”).
https://dev.twitter.com/ads/basics/response-codes#Error+Response+Structure

I am just trying to be cautious here as this is a breaking change.

@Roach any thoughts/insight here?
Original issue: #100

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Aug 4, 2015

I got reply from twitter staff and it seems to be there is a situation, where array can has more than one error =(

"errors": [
    {
      "code": "INVALID",
      "message": "LineItems with different placement types are not allowed inside a campaign : 3222623",
      "attribute": "placement_type"
    },
    {
      "code": "INVALID",
      "message": "Line items with different objectives are not allowed under the same campaign (campaign_id=3222623)",
      "attribute": "objective"
    }
  ],
  "request": {
    "params": {
      "placements": [
        "ALL_ON_TWITTER"
      ],
      "bid_amount_local_micro": 350000,
      "product_type": "PROMOTED_TWEETS",
      "objective": "TWEET_ENGAGEMENTS",
      "account_id": "5gvk9h",
      "include_sentiment": "ALL",
      "campaign_id": "1x2lb"
    }
  }
}

iamstarkov commented Aug 4, 2015

I got reply from twitter staff and it seems to be there is a situation, where array can has more than one error =(

"errors": [
    {
      "code": "INVALID",
      "message": "LineItems with different placement types are not allowed inside a campaign : 3222623",
      "attribute": "placement_type"
    },
    {
      "code": "INVALID",
      "message": "Line items with different objectives are not allowed under the same campaign (campaign_id=3222623)",
      "attribute": "objective"
    }
  ],
  "request": {
    "params": {
      "placements": [
        "ALL_ON_TWITTER"
      ],
      "bid_amount_local_micro": 350000,
      "product_type": "PROMOTED_TWEETS",
      "objective": "TWEET_ENGAGEMENTS",
      "account_id": "5gvk9h",
      "include_sentiment": "ALL",
      "campaign_id": "1x2lb"
    }
  }
}
@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Aug 4, 2015

suggesting new pull-request with

data.errors = data.errors.map(err => new Error(`Status Code ${error.code}:  ${error.message}`))

and changing docs to show that passed errors can be array instead of single error

iamstarkov commented Aug 4, 2015

suggesting new pull-request with

data.errors = data.errors.map(err => new Error(`Status Code ${error.code}:  ${error.message}`))

and changing docs to show that passed errors can be array instead of single error

@desmondmorris

This comment has been minimized.

Show comment
Hide comment
@desmondmorris

desmondmorris Aug 4, 2015

Owner

@iamstarkov thanks for digging into this. +1 to your suggested solution.

Owner

desmondmorris commented Aug 4, 2015

@iamstarkov thanks for digging into this. +1 to your suggested solution.

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Aug 4, 2015

while, im not implement it, i want to ask you as a maintainer one more question. this change seems to be breaking change (somebody prob already relying on array of plain error object). is it acceptable or it doesnt worth it?

iamstarkov commented Aug 4, 2015

while, im not implement it, i want to ask you as a maintainer one more question. this change seems to be breaking change (somebody prob already relying on array of plain error object). is it acceptable or it doesnt worth it?

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Sep 23, 2015

After more work on twitter related modules, I strictly convinced that array of errors is the worst thing about this situation. I faced it so many times, since 5 august. Also, I’m sold with "Worst case is there are more than one error and the first request you see error A, fix it for the second request and then see error B." and don’t sold with official answer "it can happen in real world", because:

  1. sure it can, but then you just need to fix one issue after another
  2. while its okay for api, its not okay for nodejs library, because everybody relies on one error.

Special cases aren't special enough to break the rules.
— The Zen of Python, https://www.python.org/dev/peps/pep-0020/

as a summary I want to suggest to merge this pull-request and release new major version. What do you think?

iamstarkov commented Sep 23, 2015

After more work on twitter related modules, I strictly convinced that array of errors is the worst thing about this situation. I faced it so many times, since 5 august. Also, I’m sold with "Worst case is there are more than one error and the first request you see error A, fix it for the second request and then see error B." and don’t sold with official answer "it can happen in real world", because:

  1. sure it can, but then you just need to fix one issue after another
  2. while its okay for api, its not okay for nodejs library, because everybody relies on one error.

Special cases aren't special enough to break the rules.
— The Zen of Python, https://www.python.org/dev/peps/pep-0020/

as a summary I want to suggest to merge this pull-request and release new major version. What do you think?

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Sep 24, 2015

@desmondmorris what do you think about it?

iamstarkov commented Sep 24, 2015

@desmondmorris what do you think about it?

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov commented Sep 30, 2015

@desmondmorris polite ping)

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Oct 5, 2015

hi, should I close it? (I’m still hoping to get it merged)

iamstarkov commented Oct 5, 2015

hi, should I close it? (I’m still hoping to get it merged)

@desmondmorris

This comment has been minimized.

Show comment
Hide comment
@desmondmorris

desmondmorris Oct 6, 2015

Owner

@iamstarkov sorry for the delay. I think your argument here is def worth the consideration. I will take a look at this.

Owner

desmondmorris commented Oct 6, 2015

@iamstarkov sorry for the delay. I think your argument here is def worth the consideration. I will take a look at this.

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Oct 9, 2015

@desmondmorris it’s okay. hopefully, you will have time for this pull-request now.

waiting to develop bunch of new modules dependent on node-twitter this weekend or definitely in the start of next week

iamstarkov commented Oct 9, 2015

@desmondmorris it’s okay. hopefully, you will have time for this pull-request now.

waiting to develop bunch of new modules dependent on node-twitter this weekend or definitely in the start of next week

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Oct 11, 2015

any news? =)

iamstarkov commented Oct 11, 2015

any news? =)

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov Oct 15, 2015

for now I switched my packages to twit — it handles errors in proper way

iamstarkov commented Oct 15, 2015

for now I switched my packages to twit — it handles errors in proper way

@iamstarkov

This comment has been minimized.

Show comment
Hide comment
@iamstarkov

iamstarkov commented Dec 3, 2015

okay

@iamstarkov iamstarkov closed this Dec 3, 2015

@reconbot reconbot reopened this May 14, 2016

@iamstarkov iamstarkov closed this May 14, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment