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

module: remove unnecessary JSON.stringify #3578

Closed
wants to merge 1 commit into from
Closed

module: remove unnecessary JSON.stringify #3578

wants to merge 1 commit into from

Conversation

zertosh
Copy link
Contributor

@zertosh zertosh commented Oct 29, 2015

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
Copy link
Contributor

could template strings have a similar result?

@cjihrig
Copy link
Contributor

cjihrig commented Oct 29, 2015

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
Copy link
Contributor Author

zertosh commented Oct 29, 2015

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

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Oct 29, 2015
debug('RELATIVE: requested:' + request +
' set ID to: ' + id + ' from ' + parent.id);
debug('RELATIVE: requested: %s set ID to: %s from %s', request, id,
parent.id);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

`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
Copy link
Contributor

LGTM with a style nit

@evanlucas
Copy link
Contributor

LGTM

@jasnell
Copy link
Member

jasnell commented Oct 29, 2015

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

@trevnorris
Copy link
Contributor

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

@zertosh
Copy link
Contributor Author

zertosh commented Oct 31, 2015

Seems like everyone is Ok on this. Merge?

@evanlucas
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 5, 2015

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

@cjihrig
Copy link
Contributor

cjihrig commented Nov 5, 2015

Although, has CI been run?

@evanlucas
Copy link
Contributor

@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

+1 :)
On Nov 5, 2015 6:38 AM, "Colin Ihrig" [email protected] 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
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Nov 5, 2015

CI was green. Landed in faa3bb8

@jasnell jasnell closed this Nov 5, 2015
rvagg pushed a commit that referenced this pull request Nov 7, 2015
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@Fishrock123 Fishrock123 mentioned this pull request Nov 11, 2015
MylesBorins pushed a commit that referenced this pull request Nov 16, 2015
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

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

rvagg pushed a commit that referenced this pull request Dec 4, 2015
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell jasnell mentioned this pull request Dec 17, 2015
jasnell pushed a commit that referenced this pull request Dec 17, 2015
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
jasnell pushed a commit that referenced this pull request Dec 23, 2015
`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 <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants