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

added error handling override function #181

Merged
merged 1 commit into from Aug 26, 2016
Merged

Conversation

sinkingrowboats
Copy link

No description provided.

@boxcla
Copy link

boxcla commented Aug 9, 2016

Verified that @sinkingrowboats has signed the CLA. Thanks for the pull request!

@zephraph
Copy link
Contributor

zephraph commented Aug 9, 2016

@j3tan I'm assuming that this is going to fail due to the eslint rule no-function-assign. Is this approach acceptable? Should we do something different?

@zephraph
Copy link
Contributor

@j3tan Could you review? I suspect you'll want us to change this approach in favor of one that doesn't do function reassignment how it was approached here.

* @returns {void}
*/
setErrorHandler: function(exceptionHandler) {
error = exceptionHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's the approach I would take:

Create a variable at the top of this file, next to initialized = false;

var customErrorHandler;

then you could do:
customErrorHandler = exceptionHandler;

then do:

function error(exception) {
    if (customErrorHandler) {
         customErrorHandler(exception);
         return;
    }

    if (globalConfig.debug) ...
}

This will maintain the integrity of Box.Application while still giving you a handle/callback

@zephraph
Copy link
Contributor

Failing due to test coverage, I'll add those sometime today.

@j3tan
Copy link
Contributor

j3tan commented Aug 17, 2016

Code looks good. You can even be safer and do the following:

if (typeof customErrorHandler === 'function') {
   ...
}

@j3tan
Copy link
Contributor

j3tan commented Aug 17, 2016

please include tests, then we can merge this in!

@zephraph
Copy link
Contributor

Added a test. Didn't really stress test all of the use cases, though I can if you want. We should also probably add some extra documentation around this.

@j3tan
Copy link
Contributor

j3tan commented Aug 17, 2016

Last request: please link to issue ticket # in commit description

@zephraph
Copy link
Contributor

On every commit? Or do you want us to squash and have one commit with the ticket number in it?

@j3tan
Copy link
Contributor

j3tan commented Aug 17, 2016

Squash is preferable, helps with commit history

@zephraph
Copy link
Contributor

@j3tan I squashed everything. Could you give it one last look over?

Thanks!

@j3tan j3tan merged commit 74c62ad into box:master Aug 26, 2016
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.

None yet

4 participants