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

refactor: change error message format to multiline #362

Merged
merged 4 commits into from
Feb 6, 2019
Merged

refactor: change error message format to multiline #362

merged 4 commits into from
Feb 6, 2019

Conversation

callmehiphop
Copy link
Contributor

@callmehiphop callmehiphop commented Feb 4, 2019

Fixes googleapis/nodejs-bigquery#326

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Using the following server error for the before/after

{
  "state": "DONE",
  "errorResult": {
    "reason": "invalid",
    "location": "",
    "message": "Error while reading data, error message: JSON table encountered too many errors, giving up. Rows: 1; errors: 1. Please look into the errors[] collection for more details."
  },
  "errors": [
    {
      "reason": "invalid",
      "location": "",
      "message": "Error while reading data, error message: JSON table encountered too many errors, giving up. Rows: 1; errors: 1. Please look into the errors[] collection for more details."
    },
    {
      "reason": "invalid",
      "location": "",
      "message": "Error while reading data, error message: JSON parsing error in row starting at position 0: Could not convert value to string. Field: caller; Value: 822"
    }
  ]
}

Before

Error while reading data, error message: JSON table encountered too many errors, giving up. Rows: 1; errors: 1. Please look into the errors[] collection for more details.
    at endReadableNT (_stream_readable.js:1081:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

After

Multiple errors occurred during the request. Please see the `errors` array for complete details.

    1. Error while reading data, error message: JSON table encountered too many errors, giving up. Rows: 1; errors: 1. Please look into the errors[] collection for more details.
    2. Error while reading data, error message: JSON parsing error in row starting at position 0: Could not convert value to string. Field: caller; Value: 822

    at endReadableNT (_stream_readable.js:1081:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 4, 2019
@JustinBeckwith
Copy link
Contributor

Is there any way this could be breaking?

@callmehiphop
Copy link
Contributor Author

@JustinBeckwith I think if users did any kinds of string/regexp operations against the error message, it would definitely throw them for a loop. If preferable, I could just make some new custom errors in the BigQuery lib itself, but it would still be a breaking change in the same capacity, which would require a semver major bump.

@stephenplusplus
Copy link
Contributor

Can you do a before / after?

@callmehiphop
Copy link
Contributor Author

@stephenplusplus if you go to the linked issue, there's a pretty good diff on the two.

@stephenplusplus
Copy link
Contributor

I didn't see it. Could you do one for here anyway in the opening post so it's easier for users and future us? :)

@callmehiphop
Copy link
Contributor Author

@stephenplusplus I just updated the overview with some examples. PTAL!

@stephenplusplus
Copy link
Contributor

This does make it easier to read, but since we're here, we could probably kick it up a notch:

Multiple errors occurred during the request. Please see the `errors` array for complete details.

    1. Error while reading data, error message: JSON table encountered too many errors, giving up. Rows: 1; errors: 1. Please look into the errors[] collection for more details.
    2. Error while reading data, error message: JSON parsing error in row starting at position 0: Could not convert value to string. Field: caller; Value: 822

    at endReadableNT (_stream_readable.js:1081:12)
    at process._tickCallback (internal/process/next_tick.js:63:19)

@callmehiphop
Copy link
Contributor Author

@stephenplusplus SGTM - PTAL :)

@stephenplusplus
Copy link
Contributor

@callmehiphop would you mind updating the first post with the new formatting?

@callmehiphop
Copy link
Contributor Author

@stephenplusplus sure, done!

@stephenplusplus stephenplusplus merged commit 64c585f into googleapis:master Feb 6, 2019
@oliverwoodings
Copy link

oliverwoodings commented Feb 6, 2019

Thanks so much for getting this done! Love how quickly you guys pick up and resolve issues. 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants