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

test: remove common.inspect() #3257

Closed
wants to merge 2 commits into from
Closed

test: remove common.inspect() #3257

wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 8, 2015

common.inspect() is just util.inspect(). common copies every property
from util but this is the only one that gets used and it only gets used
in three files. Well, four, but the fourth is removed in a pending #3256.

This commit removes it. Subsequently, the "copy util to common"
part of common can be removed altogether.

common.inspect() is just util.inspect(). common copies every property
from util but this is the only one that gets used and it only gets used
in three places. Well, four, but the fourth is removed in a pending PR.

This commit removes it. Subsequently, the "copy util to common"
part of `common` can be removed altogether.
@Trott Trott added the test Issues and PRs related to the tests. label Oct 8, 2015
@thefourtheye
Copy link
Contributor

I think we can remove those strings altogether. They may be used almost never and the tests might speed up a little. Also why not remove it from common?

@Trott
Copy link
Member Author

Trott commented Oct 8, 2015

@thefourtheye Removing the strings entirely is a good idea, I think. I'll do that. Thanks for the suggestion.

Can't remove it from common until both this and #3256 land.

@Trott
Copy link
Member Author

Trott commented Oct 8, 2015

@thefourtheye
Copy link
Contributor

LGTM if the CI is green.

@evanlucas
Copy link
Contributor

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Oct 9, 2015
common.inspect() is just util.inspect(). common copies every property
from util but this is the only one that gets used and it only gets used
in three places. Well, four, but the fourth is removed in a pending PR.

This commit removes it. Subsequently, the "copy util to common"
part of `common` can be removed altogether.

PR-URL: nodejs#3257
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@Trott
Copy link
Member Author

Trott commented Oct 9, 2015

Landed in 72c8a74.

@Trott Trott closed this Oct 9, 2015
Trott added a commit that referenced this pull request Oct 10, 2015
common.inspect() is just util.inspect(). common copies every property
from util but this is the only one that gets used and it only gets used
in three places. Well, four, but the fourth is removed in a pending PR.

This commit removes it. Subsequently, the "copy util to common"
part of `common` can be removed altogether.

PR-URL: #3257
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2015

Landed in v4.x in f0f8afd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants