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

Modify RestAdapter to allow disabling errorHandler #248

Merged
merged 1 commit into from Oct 21, 2015

Conversation

digitalsadhu
Copy link
Contributor

options.handleErrors = false will allow the RestAdapter error handler
to be bypassed

See: strongloop/loopback#445 (comment) for discussion

@bajtos does this look ok?

@slnode
Copy link

slnode commented Oct 13, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@listepo
Copy link

listepo commented Oct 13, 2015

👍

@bajtos bajtos self-assigned this Oct 15, 2015
@bajtos
Copy link
Member

bajtos commented Oct 15, 2015

@slnode ok to test

@bajtos
Copy link
Member

bajtos commented Oct 15, 2015

Hi @digitalsadhu, thank you for the pull request. Could you please add some unit-tests to verify the implementation and prevent regressions in the future? See f26a871 for an inspiration.

@digitalsadhu
Copy link
Contributor Author

@bajtos tests added, still seeing failing checks there. Not sure how I would resolve those as tests are all passing for me. Can you point me in the right direction?

@bajtos
Copy link
Member

bajtos commented Oct 15, 2015

LGTM.

@ritch would you like to take a look at this change yourself too?

still seeing failing checks there. Not sure how I would resolve those as tests are all passing for me. Can you point me in the right direction?

Those test failures are in projects that are using strong-remoting as a dependency. I think it's the loopback-example-oracle repository that is failing unit-tests because the oracle driver does not support Node v4.x yet.

@rmg any ideas how to fix this false alert?

@digitalsadhu don't worry about those dependent builds, I am ok to land the PR with loopback-example-oracle failing. Could you please squash the commits into a single one?

@digitalsadhu
Copy link
Contributor Author

@bajtos Squashed

@bajtos
Copy link
Member

bajtos commented Oct 15, 2015

@digitalsadhu a nitpick, could you please fix the commit message to use the correct option name options.handleErrors (plural, not singular)?

@bajtos
Copy link
Member

bajtos commented Oct 15, 2015

I asked @ritch to review and land this, I am on vacation tomorrow.

options.handleErrors = false will allow the RestAdapter
to be bypassed
@digitalsadhu
Copy link
Contributor Author

@bajtos @ritch Oops, fixed commit message now.

@rmg
Copy link
Member

rmg commented Oct 15, 2015

@bajtos they aren't warnings, the loopback-example-recipes module has a hard dependency on loopback-connector-oracle :-( I've opened an issue for it: strongloop/loopback-example-recipes#8

@digitalsadhu
Copy link
Contributor Author

@bajtos how are we looking?

bajtos added a commit that referenced this pull request Oct 21, 2015
Modify RestAdapter to allow disabling errorHandler
@bajtos bajtos merged commit e8420c3 into strongloop:master Oct 21, 2015
@bajtos
Copy link
Member

bajtos commented Oct 21, 2015

@digitalsadhu thanks for the reminder. Since @ritch has not commented, I'll assume he does not have any major objections.

Landed, thank you for the valuable contribution!

@ritch
Copy link
Member

ritch commented Oct 21, 2015

👍

@digitalsadhu
Copy link
Contributor Author

Yay! Thanks guys.

On Wed, 21 Oct 2015 at 15:33, Ritchie Martori notifications@github.com
wrote:

[image: 👍]


Reply to this email directly or view it on GitHub
#248 (comment)
.

@bajtos
Copy link
Member

bajtos commented Oct 21, 2015

Released to npmjs.org as strong-remoting@2.22.0

@bajtos
Copy link
Member

bajtos commented Oct 21, 2015

Enjoy :)

@crandmck
Copy link
Contributor

crandmck commented Nov 3, 2015

@digitalsadhu I'd like to add this to the API documentation on https://apidocs.strongloop.com/strong-remoting/ but I'm not sure where it should go. Can you give me a pointer?

@bajtos
Copy link
Member

bajtos commented Nov 4, 2015

@crandmck I think here is a better place where to document this new feature: https://docs.strongloop.com/display/LB/config.json?src=search#config.json-Remotingproperties

The property name is rest.handleErrors.

While you are at it, could you please document rest.handleUnknownPaths too? It was added by f26a871.

@crandmck
Copy link
Contributor

crandmck commented Nov 4, 2015

@bajtos Thank you, that does make more sense.

I added both those properties to https://docs.strongloop.com/display/LB/config.json#config.json-Remotingproperties. In particular, the descriptions could perhaps have a bit more... PTAL.

@digitalsadhu
Copy link
Contributor Author

Nice!
1 thing though, typo: "handleErros" : true,

@crandmck
Copy link
Contributor

crandmck commented Nov 4, 2015

Doh! Thanks for catching--fixed...

@bajtos
Copy link
Member

bajtos commented Nov 5, 2015

@crandmck I have added a bit more info, PTAL.

@crandmck
Copy link
Contributor

crandmck commented Nov 5, 2015

Thanks again @bajtos

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

8 participants