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: use repeat() instead of new Array().join() #11071

Closed
wants to merge 1 commit into
from

Conversation

@JacksonTian
Contributor

JacksonTian commented Jan 30, 2017

The usage of new Array(length + 1).join(str) is strange.
Change to str.repeat(length) is more clearly.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@lpinca

lpinca approved these changes Jan 30, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jan 30, 2017

Contributor

First line of the commit message is a tad too long. Perhaps use: 'test: use repeat() instead of new Array().join()' ?

Contributor

mscdex commented Jan 30, 2017

First line of the commit message is a tad too long. Perhaps use: 'test: use repeat() instead of new Array().join()' ?

@evanlucas

LGTM with Brian's nit fixed

test: use repeat() instead of new Array().join()
The usage of `new Array(length + 1).join(str)` is strange.
Change to `str.repeat(length)` is more clearly.
@JacksonTian

This comment has been minimized.

Show comment
Hide comment
@JacksonTian

JacksonTian Jan 30, 2017

Contributor

Used the shorter commit message. Thanks all.

Contributor

JacksonTian commented Jan 30, 2017

Used the shorter commit message. Thanks all.

@JacksonTian JacksonTian changed the title from test: use repeat() instead of the new Array().join() to test: use repeat() instead of new Array().join() Jan 30, 2017

@targos

targos approved these changes Jan 31, 2017

@hiroppy

hiroppy approved these changes Feb 2, 2017

@italoacasas

This comment has been minimized.

Show comment
Hide comment
Member

italoacasas commented Feb 2, 2017

Landed 58dc229

@italoacasas italoacasas closed this Feb 2, 2017

italoacasas added a commit that referenced this pull request Feb 2, 2017

test: use repeat() instead of new Array().join()
The usage of `new Array(length + 1).join(str)` is strange.
Change to `str.repeat(length)` is more clearly.

PR-URL: #11071
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>

italoacasas added a commit that referenced this pull request Feb 3, 2017

test: use repeat() instead of new Array().join()
The usage of `new Array(length + 1).join(str)` is strange.
Change to `str.repeat(length)` is more clearly.

PR-URL: #11071
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: use repeat() instead of new Array().join()
The usage of `new Array(length + 1).join(str)` is strange.
Change to `str.repeat(length)` is more clearly.

PR-URL: nodejs#11071
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Italo A. Casas <me@italoacasas.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

will require a backport PR to land on v6 or v4

Member

jasnell commented Mar 7, 2017

will require a backport PR to land on v6 or v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment