FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: remove unnecessary JSON.stringify by zertosh · Pull Request #3578 · nodejs/node · GitHub

/ node Public

module: remove unnecessary JSON.stringify - #3578

Closed
zertosh wants to merge 1 commit into
nodejs:masterfrom
zertosh:remove-stringify-in-module
ConversationCommits (1)ChecksFiles changed
Closed

module: remove unnecessary JSON.stringify#3578
zertosh wants to merge 1 commit into
nodejs:masterfrom
zertosh:remove-stringify-in-module

Conversation

zertosh commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

Use debulog's placeholder replacement %j for stringification. So calls to JSON.stringify are only made by debuglog when you're actually debugging. Also, made the remaining calls to debuglog consistent with this style.

MylesBorins commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

could template strings have a similar result?

cjihrig commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

LGTM. It's simpler to read too. I think template strings would suffer the same problem as the original code (stringification even when not debugging).

zertosh commented Oct 29, 2015

Copy link
Copy Markdown
Contributor Author

For the %s, yes – for the %j, no. %j is the perf win here.

mscdex added the module Issues and PRs related to the module subsystem. label Oct 29, 2015
thefourtheye reviewed Oct 29, 2015
View reviewed changes
Comment thread lib/module.js Outdated

thefourtheye Oct 29, 2015

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Style nit: this should align with the previous line's parameter

zertosh Oct 29, 2015

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done!

module: remove unnecessary JSON.stringify
7c7f131 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

thefourtheye commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

LGTM with a style nit

evanlucas commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Oct 29, 2015

Copy link
Copy Markdown
Member

LGTM but since this touches module we may want more @nodejs/ctc review.

trevnorris commented Oct 29, 2015

Copy link
Copy Markdown
Contributor

I generally hate how we handle debug() in core anyway, but this is a nice perf win. LGTM.

zertosh mentioned this pull request Oct 31, 2015
Closed

zertosh commented Oct 31, 2015

Copy link
Copy Markdown
Contributor Author

Seems like everyone is Ok on this. Merge?

evanlucas commented Nov 5, 2015

Copy link
Copy Markdown
Contributor

@jasnell am I good to land this or do we want to get CTC review?

cjihrig commented Nov 5, 2015

Copy link
Copy Markdown
Contributor

I think 3 CTC LGTMs and 2 more from collaborators should be sufficient.

cjihrig commented Nov 5, 2015

Copy link
Copy Markdown
Contributor

Although, has CI been run?

evanlucas commented Nov 5, 2015

Copy link
Copy Markdown
Contributor

Good catch. CI: https://ci.nodejs.org/job/node-test-pull-request/674/

jasnell commented Nov 5, 2015

Copy link
Copy Markdown
Member

+1 :)
On Nov 5, 2015 6:38 AM, "Colin Ihrig" notifications@github.com wrote:

I think 3 CTC LGTMs and 2 more from collaborators should be sufficient.

—
Reply to this email directly or view it on GitHub
#3578 (comment).

jasnell pushed a commit that referenced this pull request Nov 5, 2015
module: remove unnecessary JSON.stringify
faa3bb8 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

jasnell commented Nov 5, 2015

Copy link
Copy Markdown
Member

CI was green. Landed in faa3bb8

jasnell closed this Nov 5, 2015
Fishrock123 mentioned this pull request Nov 6, 2015
Closed
rvagg pushed a commit that referenced this pull request Nov 7, 2015
module: remove unnecessary JSON.stringify
89285db 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 mentioned this pull request Nov 11, 2015
Merged
MylesBorins pushed a commit that referenced this pull request Nov 16, 2015
module: remove unnecessary JSON.stringify
948af71 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

MylesBorins commented Nov 16, 2015

Copy link
Copy Markdown
Contributor

landed in lts-v4.x-staging as 948af71

rvagg mentioned this pull request Nov 17, 2015
Closed
rvagg pushed a commit that referenced this pull request Dec 4, 2015
module: remove unnecessary JSON.stringify
27085bf 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell mentioned this pull request Dec 17, 2015
Closed
jasnell pushed a commit that referenced this pull request Dec 17, 2015
module: remove unnecessary JSON.stringify
d80fa2c 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
module: remove unnecessary JSON.stringify
5086a9c 
`debuglog` uses `%j` as a placeholder for replacement with
`JSON.stringify`. So that `JSON.stringify` is only called when the
appropriate debug flag is on. The other `%s` changes are for style
consistency.

PR-URL: #3578
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Dec 7, 2023
Open
Open
Open
leonardoadame mentioned this pull request Dec 8, 2023
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

module Issues and PRs related to the module subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL