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.

Sign up for GitHub

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

Jump to bottom

Remove uncaught interface #2208

Merged
merged 1 commit into from
Apr 14, 2017
Merged

Remove uncaught interface #2208

merged 1 commit into from
Apr 14, 2017

Conversation

cristiancavalli
Copy link
Contributor

@cristiancavalli cristiancavalli commented Apr 11, 2017 •
edited
Loading

Blockers

Remove Uncaught Exception Handler

  • Remove the uncaught exception handler
  • Remove the corresponding tests & documentation
  • Add new documentation to README stating how feature can be recreated in application code

Note:

Commit is currently larger than actual landing-scope since it is stacked on in-flight PR #2189

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 11, 2017
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 8286972 on cristiancavalli:remove-uncaught-interface into 87312f9 on GoogleCloudPlatform:master.

ofrobots
ofrobots suggested changes Apr 11, 2017
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

I think the README.md changes are missing.

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from 8286972 to ccd0553 Compare April 11, 2017 18:23
@cristiancavalli
Copy link
Contributor Author

@ofrobots my bad -- weird rebase flow.

Changes added

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from ccd0553 to ec21135 Compare April 11, 2017 18:25
ofrobots
ofrobots reviewed Apr 11, 2017
View reviewed changes
packages/error-reporting/README.md Outdated
@@ -44,6 +44,13 @@ var errors = require('@google-cloud/error-reporting')({
errors.report(new Error('Something broke!'));
```

- *One may even catch and report application-wide uncaught errors:*

This comment was marked as spam.

Sign in to view

packages/error-reporting/README.md Outdated

```js
var errors = require('@google-cloud/error-reporting')();
process.on('uncaughtException', e => errors.report(e));

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ec21135 on cristiancavalli:remove-uncaught-interface into 87312f9 on GoogleCloudPlatform:master.

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from bfe989f to fb23ced Compare April 11, 2017 18:57
@cristiancavalli
Copy link
Contributor Author

@ofrobots updated

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling fb23ced on cristiancavalli:remove-uncaught-interface into 87312f9 on GoogleCloudPlatform:master.

@stephenplusplus
Copy link
Contributor

@ofrobots let me know when you're happy with this and I'll try to merge within 10 seconds.

ofrobots
ofrobots reviewed Apr 11, 2017
View reviewed changes
packages/error-reporting/README.md Outdated
process.on('uncaughtException', e => errors.report(e));
process.on('uncaughtException', (e) => {
// Write the error to stderr. If one does not manually do this
// nothing will be printed before the VM exits.

This comment was marked as spam.

Sign in to view

ofrobots
ofrobots reviewed Apr 11, 2017
View reviewed changes
packages/error-reporting/README.md Outdated
- *One may even catch and report application-wide uncaught errors:*
- **One may even catch and report application-wide uncaught errors:**
- *It is recommended to catch uncaughtExceptions for production-deployed applications*
- [To read more about uncaught exception handling in Node.js and what it means for your application please click here](https://nodejs.org/api/process.html#process_event_uncaughtexception)

This comment was marked as spam.

Sign in to view

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from fb23ced to 9116a94 Compare April 11, 2017 20:39
@cristiancavalli
Copy link
Contributor Author

@ofrobots updated

ofrobots
ofrobots approved these changes Apr 11, 2017
View reviewed changes
Copy link
Contributor

@ofrobots ofrobots left a comment

Choose a reason for hiding this comment

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

LGTM. Is this blocked on #2189?

@cristiancavalli
Copy link
Contributor Author

@ofrobots yes these commits are stacked on top of #2189

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 9116a94 on cristiancavalli:remove-uncaught-interface into 184f9bb on GoogleCloudPlatform:master.

@ofrobots ofrobots mentioned this pull request Apr 12, 2017
Merged
6 tasks
@stephenplusplus stephenplusplus added api: error-reporting status: blocked Resolving the issue is dependent on other work. labels Apr 13, 2017
@stephenplusplus
Copy link
Contributor

Is this really over?

@cristiancavalli
Copy link
Contributor Author

No not yet, sorry was trying to see if closing this had an effect on the PR file propagation for the CI integration

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from 9116a94 to 6c1e4b4 Compare April 13, 2017 20:57
@cristiancavalli
Copy link
Contributor Author

@stephenplusplus @ofrobots merge conflicts resolved!

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 6c1e4b4 on cristiancavalli:remove-uncaught-interface into f0812b6 on GoogleCloudPlatform:master.

@cristiancavalli cristiancavalli force-pushed the remove-uncaught-interface branch from 6c1e4b4 to 54a6ead Compare April 13, 2017 21:21
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 54a6ead on cristiancavalli:remove-uncaught-interface into f0812b6 on GoogleCloudPlatform:master.

@stephenplusplus stephenplusplus removed the status: blocked Resolving the issue is dependent on other work. label Apr 14, 2017
@stephenplusplus stephenplusplus merged commit 326ad28 into googleapis:master Apr 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephenplusplus stephenplusplus stephenplusplus approved these changes

@ofrobots ofrobots ofrobots approved these changes

Assignees
No one assigned
Labels
api: error-reporting cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cristiancavalli @coveralls @stephenplusplus @ofrobots @googlebot