tapjs / stack-utils

Captures and cleans stack traces.
https://www.npmjs.com/package/stack-utils
MIT License
190 stars 35 forks source link

Alter parseLine regex to allow `(` character within location. #24

Closed jaridmargolin closed 7 years ago

jaridmargolin commented 7 years ago

PR to open discussion regarding #23, Fail to parseLine when path contains '('.

Example:

stackUtils.parseLine('    at Context.<anonymous> (/Users/USER/Dropbox (Personal)/example/test.js:14:11)')
// => null

stackUtils.parseLine('    at Context.<anonymous> (/Users/USER/Dropbox/example/test.js:14:11)')
// => { line: 14, column: 11, file: '/Users/USER/Dropbox/example/test.js', function: 'Context.<anonymous>' }

This may be a naive approach, but simply changing the matching pattern for location to not negate ) appears to fix the issue while not breaking any of the existing tests. However, negating the character in the first place appears to have been a very intentional choice. I am assuming there is a missing testcase for this requirement?

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 98.63% when pulling 6e666109d21d5e3010f86033108fbe6749b260fc on jaridmargolin:master into bf7bf375c64c28b42b6c74034ac78b9e2a55dd45 on tapjs:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 98.63% when pulling 6e666109d21d5e3010f86033108fbe6749b260fc on jaridmargolin:master into bf7bf375c64c28b42b6c74034ac78b9e2a55dd45 on tapjs:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 98.63% when pulling 6e666109d21d5e3010f86033108fbe6749b260fc on jaridmargolin:master into bf7bf375c64c28b42b6c74034ac78b9e2a55dd45 on tapjs:master.

jaridmargolin commented 7 years ago

Regarding tests... All tests on my machine fail without this patch because the path at which I am working on this PR contains a ( character. With the patch added, I see nothing but green :)

If executed in a different cwd, on CI for instance, this patch will not be properly tested. If I can get buy-in that this is a valid approach to fixing the problem, and I am not overlooking any obvious shortcomings, I will add a more robust test to guarantee this patch is operating as intended.

@jamestalmage @isaacs

jaridmargolin commented 7 years ago

Thanks!