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

util: mark cwd grey while inspecting errors #41082

Closed
wants to merge 2 commits into from

Conversation

BridgeAR
Copy link
Member

@BridgeAR BridgeAR commented Dec 4, 2021

This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Old:
image

New:
image

Signed-off-by: Ruben Bridgewater [email protected]

@nodejs-github-bot nodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Dec 4, 2021
@mscdex
Copy link
Contributor

mscdex commented Dec 4, 2021

Is grey currently only used for stack trace lines that point to node internals? If so, perhaps we should use a different color for cwd to avoid confusion?

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 4, 2021

@mscdex

Is grey currently only used for stack trace lines that point to node internals?

Yes, currently only node internal stack frames are grey. It was meant as "this is probably not important" (no matter what it represented), focus on the rest.

If so, perhaps we should use a different color for cwd to avoid confusion?

Do you have a concrete suggestion? I fear using colors would actually highlight the cwd instead of moving it to the background.

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

I am wondering if it would be better to use the directory containing the first package.json instead. The cwd is not necessarily where the code is (even though it probably is most of the time).

lib/events.js Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 4, 2021

@tniessen

I am wondering if it would be better to use the directory containing the first package.json instead. The cwd is not necessarily where the code is (even though it probably is most of the time).

This would require util.inspect() to read files in directories sync and that is something I would rather not do. If code is run somewhere that has nothing to do with the cwd, the whole path is still visible. If it is the cwd, it's probably fine to just move it to the background as it's something "already known", no matter if the code is run there or not (some code has to be run there, otherwise it's not going to be marked grey).

@tniessen
Copy link
Member

tniessen commented Dec 4, 2021

@tniessen

I am wondering if it would be better to use the directory containing the first package.json instead. The cwd is not necessarily where the code is (even though it probably is most of the time).

This would require util.inspect() to read files in directories sync and that is something I would rather not do. If code is run somewhere that has nothing to do with the cwd, the whole path is still visible. If it is the cwd, it's probably fine to just move it to the background as it's something "already known", no matter if the code is run there or not (some code has to be run there, otherwise it's not going to be marked grey).

I totally get that, and I don't object to using the working directory. I just thought we are probably reading the package.json anyway at some point (e.g., to check if it's a "type": "module") so we might as well use that directory.

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
lib/internal/util/inspect.js Show resolved Hide resolved
lib/internal/util/inspect.js Show resolved Hide resolved
lib/internal/util/inspect.js Show resolved Hide resolved
lib/internal/util/inspect.js Show resolved Hide resolved
lib/internal/util/inspect.js Show resolved Hide resolved
@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2021

@tniessen I just tried to figure out how to implement something like that and could not think of a good way to do so. We do not always have a package.json or have multiple ones in different sub directories, in what case I would only want to grey out the main part, not the sub directory for the executed code. Could you give me a hint how you would implement something like that and how it would behave for the mentioned situations? That would help :-)

@tniessen
Copy link
Member

tniessen commented Dec 6, 2021

@BridgeAR That does make it difficult, you are right. I am not too worried about using cwd, we can probably change the exact conditions later without breaking anything (if we ever want to).

We could, maybe, determine a prefix of all loaded JS modules, but that seems out of scope here, and I am not even sure if that would be reliable.

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 6, 2021
@nodejs-github-bot
Copy link
Collaborator

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 6, 2021

@tniessen I agree, we can definitely improve the implementation later. Are you fine with the change as is in general?

@nodejs-github-bot
Copy link
Collaborator

@tniessen
Copy link
Member

tniessen commented Dec 6, 2021

Are you fine with the change as is in general?

It seems reasonable to me. The only super tiny nit I have is that, personally, I believe that the first / should be gray as well (e.g., highlight test.js instead of /test.js) because the highlighted part of the path should be relative to cwd, not an absolute path.

@BridgeAR
Copy link
Member Author

BridgeAR commented Dec 7, 2021

I updated it to be a relative path:

image

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 7, 2021
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2022
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Jul 9, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 2fe4e94...7cfd5f1

nodejs-github-bot pushed a commit that referenced this pull request Jul 9, 2022
This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Jul 9, 2022
Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 12, 2022
Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 20, 2022
This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 20, 2022
Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
targos pushed a commit that referenced this pull request Jul 31, 2022
Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: #41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
This changes the util.inspect() output for errors in case stack
traces contain the current working directory in their trace.
If that's the case, the cwd part is marked grey to focus on the
rest of the path.

Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: nodejs/node#41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
Signed-off-by: Ruben Bridgewater <[email protected]>

PR-URL: nodejs/node#41082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. events Issues and PRs related to the events subsystem / EventEmitter. needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants