-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Better string representation of errors #2712
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
client/karma.js
Outdated
|
||
// create an object with the string representation of the message to ensure all its content is properly | ||
// transferred to the console log | ||
message = {message: message, str: message.toString().replace(/\t/g, " ").split("\n")} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The replace
and split
parts should probably be removed.
Also, this line changes the message variable's type from string to an object - this probably means that tests have to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the replace and split parts are for nicer output display in the console, since the object is converted to json.
With the split and replace, you get
"str": [
"Error: (SystemJS) Object prototype may only be an Object or null: undefined",
" TypeError: Object prototype may only be an Object or null: undefined",
" at setPrototypeOf (<anonymous>)",
" at __extends$1 (dist/bundles/my-bundle.umd.js:2258:9)",
]
instead of
"str": "Error: (SystemJS) Object prototype may only be an Object or null: undefined\n\tTypeError: Object prototype may only be an Object or null: undefined\n\tat setPrototypeOf (<anonymous>)\n\tat __extends$1 (dist/bundles/my-bundle.umd.js:2258:9)",
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the variable type is not changed, the message already is an object that will later on be converted to a string with JSON.stringify()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the message might be a string or an object
@wesleycho could you have a look at this again? this patch really helped me a lot when investigating preliminary import / systemjs issues that prevented the tests from being run and log a useful message. |
@wesleycho could you please have a look at this again? |
I would also highly appreciate if this change is going to be integrated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @wesleycho, I’d say the error should remain a string. Formatting of the message itself should be left to the client console or to formatting methods, instead of having the message be preformatted when attached to the object.
@EzraBrooks @wesleycho I can understand the formatting should not be done here. but since the message can be an object (which is the reason for this PR), it should be ok to transform it to a different object as I suggest in this PR. would you accept this PR if I removed the |
Thanks, I’ll approve it once the CI checks complete. |
@EzraBrooks it seems there is a problem with the travis ci build retrieving some commit message. however, all other tests pass. could you please have a look at it? |
Please squash your commits into one that follows Karma’s naming conventions - Travis is failing on commit message validation. |
Sorry, to clarify.. the commit messages all have a leading type on them. In this case, I would recommend |
@EzraBrooks done, waiting for the CI to complete |
Looks like ESLint has a few style complaints. |
@EzraBrooks I updated the PR, hope it is fine now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
Resolves #2711
Before:
After: