Skip to content

lib: remove unnecessary string interpolation - #20890

Closed
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:lib_warning_unnecessary_string_interpolation
Closed

lib: remove unnecessary string interpolation#20890
danbev wants to merge 1 commit into
nodejs:masterfrom
danbev:lib_warning_unnecessary_string_interpolation

Conversation

@danbev

Copy link
Copy Markdown
Contributor
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the process Issues and PRs related to the process subsystem. label May 22, 2018
@danbev

Copy link
Copy Markdown
ContributorAuthor

@targostargos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess the same can be done with "msg += ${warning.stack};" a few lines after?

@trivikrtrivikr added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 22, 2018
@benjamingr

Copy link
Copy Markdown
Member

I guess the same can be done with "msg += ${warning.stack};" a few lines after?

Are we sure warning.stack warning.stack is a string?

@targos

Copy link
Copy Markdown
Member

@benjamingr

Are we sure warning.stack warning.stack is a string?

We're not, but I'm not aware of any difference between stringification from concatenation and using a template literal. It doesn't have to be changed in this PR though, there are enough LGTMs for the current changes.

@danbev

Copy link
Copy Markdown
ContributorAuthor

Landed in 03043e2.

@danbevdanbev closed this May 24, 2018
@danbev
danbev deleted the lib_warning_unnecessary_string_interpolation branch May 24, 2018 08:20
danbev added a commit that referenced this pull request May 24, 2018
PR-URL: #20890
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this pull request May 25, 2018
PR-URL: #20890
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jackson Tian <shyvo1987@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request May 29, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.processIssues and PRs related to the process subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

11 participants

@danbev@benjamingr@targos@apapirovski@JacksonTian@jasnell@lpinca@cjihrig@tniessen@trivikr@nodejs-github-bot