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

What should be considered "internal"? #8

Closed
jamestalmage opened this issue Jan 13, 2016 · 4 comments
Closed

What should be considered "internal"? #8

jamestalmage opened this issue Jan 13, 2016 · 4 comments

Comments

@jamestalmage
Copy link
Member

The current list of regular-expressions doesn't capture everything I think it should.

  1. The internal folder in Nodejs.

      /\(internal\/[\w_-]+\.js:[0-9]+:[0-9]+\)$/

    Note this matches the opening (, so it should not match user folders called internal.

  2. Any native methods.

      /\(native\)$/

    Without this, I was getting long-stack-output with the following:

    From previous event:
      Array.map(native)
    

    Which is pretty much useless.

  3. Spawn wrap

     /[\\\/]\.node-spawn-wrap-\w+-\w+[\\\/]node:[0-9]+:[0-9]+\)?$/

    My thought is the only people who need to see spawn wrap output is spawn wrap developers. It doesn't technically qualify as an Nodejs internal, but it's wrapping/replacing internals, and it seems really unlikely to benefit anyone.

  4. node.js

      /node\.js:[0-9]+:[0-9]+\)?$/

    This was appearing at the tail end of a lot of stack-traces in long-stack-trace test suite. I think it should probably match the opening ( like 1.

@vadimdemedes
Copy link

What if we go for whitelist approach? Consider "internal" everything but project's folder (excluding node_modules, ofc).

@jamestalmage
Copy link
Member Author

I think that should be up to the individual test runner. All of node_modules doesn't help trace down bugs in the modules I use (or help me understand how I'm misusing them).

If they want to eliminate node_modules that's an easy one for them to add. I want StackUtils.nodeInternals() to return Node internal stuff so users of this module don't need to spend a lot of time figuring out the different forms those internals take.

The spawn-wrap suggestion is an exception to that rule. But one I thought worth debating (especially since nyc adds it to the stack).

@jamestalmage
Copy link
Member Author

I just don't want to go overboard. It hurts users more to hide a valuable line than it does to repeatedly show them a useless one. I think we should only be deleting lines if we are near certain it's unimportant to the user.

@sindresorhus
Copy link

👍 for hiding all your suggested lines.

jamestalmage added a commit that referenced this issue Jan 14, 2016
Also swaps `[0-9]` for `\d` everywhere.

Fixes #8.
jamestalmage added a commit that referenced this issue Jan 14, 2016
Also swaps `[0-9]` for `\d` everywhere.

Fixes #8.
This issue was closed.
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

No branches or pull requests

4 participants
@sindresorhus @vadimdemedes @jamestalmage and others