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

Don't expect column number to always exists #17

Merged
merged 1 commit into from
May 15, 2017

Conversation

kjg
Copy link
Collaborator

@kjg kjg commented May 11, 2017

I'm running some node code on apigee which uses rhino under the hood as the javascript engine.

Unfortunately Rhino does not include column numbers in their stack traces which currently causes stack trace parsing with this module to fail.

I've filed the following Issues:

but I'm not counting on anything happening there.

This PR make the presence of a column number in the stack trace optional to support runtimes that don't output a column number.

@felixge
Copy link
Owner

felixge commented May 12, 2017

This module was meant to only deal with v8/node stack traces, so I'm somewhat hesitant to add support for more engines. Your patch looks minimally invasive, so I think it's probably fine.

Anyway, if you can confirm that all other tests are still passing, I'll go ahead and merge.

@kjg
Copy link
Collaborator Author

kjg commented May 12, 2017

Thanks for being so willing to accept this change!

I ran into some failures on master before making my changes. They all seem to be related to specific lines being at a different level in the stack than what the test is expecting. I'm assuming this has to do with the specific version of node and other environment differences with my setup. However as far as I could tell, the new test I'm adding failed before the code change and passes after, and all the other tests that were passing in master are still passing in this branch.

@felixge felixge merged commit 962ed6a into felixge:master May 15, 2017
@felixge
Copy link
Owner

felixge commented May 15, 2017

@kjg ok. Yeah, unfortunately this repo never got hooked up to travis or similar, so I'm not sure when stuff started to break. Anyway, thx for the patch, I just merged it in.

@kjg kjg deleted the optional_column_number branch May 15, 2017 18:44
@kjg kjg restored the optional_column_number branch May 15, 2017 18:44
@kjg
Copy link
Collaborator Author

kjg commented May 15, 2017

Would you mind publishing a new version to npm? Thanks!

@felixge
Copy link
Owner

felixge commented May 16, 2017

9a11c52

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.

2 participants