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

fix: fixed parsing for node #66

Merged

Conversation

Aleksey28
Copy link
Contributor

Description

  1. added pre-replace unnecessary part of the line instead of slicing it after splitting
  2. fixed removing last round bracket during throwing out eval information
  3. removed regexp group ':(\d+):(\d+)' because we should get any location in brackets
  4. removed splitting 'sanitizedLine' and reorganized using 'sanitizedLine' and 'location'
  5. added a new option for node error test when the function name isn't there

Motivation and Context

I have a problem with the Node.js product. In some ways error looks like this:

Error
    at /var/app/scratch/my project/index.js:2:9
    at Object.<anonymous> (/var/app/scratch/my project/index.js:2:9)
    at Module._compile (internal/modules/cjs/loader.js:774:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:785:10)
    at Module.load (internal/modules/cjs/loader.js:641:32)
    at Function.Module._load (internal/modules/cjs/loader.js:556:12)
    at Function.Module.runMain (internal/modules/cjs/loader.js:837:10)
    at internal/main/run_main_module.js:17:11

First-line doesn't have a function name and round brackets and it loses full file name after splitting.

How Has This Been Tested?

Added new option for test 'should handle spaces in Node.js stacks'

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • node_modules/.bin/jscs -c .jscsrc error-stack-parser.js passes without errors
  • npm test passes without errors
  • I have read the contribution guidelines
  • I have updated the documentation accordingly
  • I have added tests to cover my changes

@niftylettuce
Copy link
Contributor

@Aleksey28 I wanted to follow up - I haven't had a chance to closely review this - but I would love to merge. Is this ready to go? I see you haven't checked off two checkboxes in the checklist above (docs and running that script without errors).

@niftylettuce niftylettuce requested review from bengourley, eriwen, oliversalzburg, victor-homyakov and a team and removed request for bengourley January 17, 2022 23:41
Copy link
Contributor

@niftylettuce niftylettuce left a comment

Choose a reason for hiding this comment

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

See my comment (you didn't check off two checkboxes yet in the GH issue template).

@Aleksey28
Copy link
Contributor Author

@niftylettuce Yes, it's ready to go. I haven't checked because they're unnecessary and they just were in the template of the PR. I think it outdated checkboxes

  • node_modules/.bin/jscs -c .jscsrc error-stack-parser.js passes without errors - I think it's outdated checkbox. The project now has ESLint instead of this package and also the project doesn't have this package in dependencies
  • I have updated the documentation accordingly - my bugfix doesn't need any documentation updates

@niftylettuce niftylettuce merged commit 1c9261a into stacktracejs:master Feb 2, 2022
@niftylettuce
Copy link
Contributor

Ping @eriwen for npm owner access so I can publish v2.0.7 via np

@niftylettuce
Copy link
Contributor

v2.0.7 published, thanks again @eriwen 🙏 cc @Aleksey28

https://github.com/stacktracejs/error-stack-parser/releases/tag/v2.0.7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants