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.

Sign up for GitHub

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

Jump to bottom

lib,test,benchmark: simplify code with String.prototype.repeat() #5359

Closed
wants to merge 1 commit into from
Closed

lib,test,benchmark: simplify code with String.prototype.repeat() #5359

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

use String.prototype.repeat() to simplify code, less code, more
semantically.

@JacksonTian
Copy link
Contributor Author

like #5311 .

@mscdex
Copy link
Contributor

mscdex commented Feb 22, 2016

CI: https://ci.nodejs.org/job/node-test-pull-request/1715/

@mscdex mscdex added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 22, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 22, 2016

Linter failed:

/usr/home/iojs/build/workspace/node-test-linter/test/parallel/test-buffer.js
  684:7  error  's' is never modified, use 'const' instead  prefer-const

@mscdex
Copy link
Contributor

mscdex commented Feb 22, 2016

Also this test is failing on all platforms:

not ok 170 test-debugger-util-regression.js
# 
# assert.js:89
#   throw new assert.AssertionError({
#   ^
# AssertionError: the program should not hang
#     at Object.exports.fail (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/common.js:453:10)
#     at fail [as _onTimeout] (/home/iojs/build/workspace/node-test-commit-linux/nodes/centos5-32/test/parallel/test-debugger-util-regression.js:23:10)
#     at Timer.listOnTimeout (timers.js:92:15)

@JacksonTian JacksonTian force-pushed the repeat branch 2 times, most recently from 00753aa to 34c6b03 Compare February 22, 2016 09:09
@JacksonTian
Copy link
Contributor Author

Hi @mscdex , I updated the PR.

@mscdex
Copy link
Contributor

mscdex commented Feb 22, 2016

CI again: https://ci.nodejs.org/job/node-test-pull-request/1717/

@mscdex
Copy link
Contributor

mscdex commented Feb 22, 2016

CI is all green

@targos
Copy link
Member

targos commented Feb 22, 2016

LGTM

@JacksonTian
lib,test,benchmark: simplify code with String.prototype.repeat()
08130dd 
use String.prototype.repeat() to simplify code, less code, more
semantically.
@JacksonTian
Copy link
Contributor Author

Rebased with master.

@jasnell
Copy link
Member

jasnell commented Mar 2, 2016

LGTM

@ronkorving ronkorving mentioned this pull request Mar 7, 2016
Closed
2 tasks
jasnell pushed a commit that referenced this pull request Mar 21, 2016
@JacksonTian @jasnell
lib: simplify code with String.prototype.repeat()
4d78121 
use String.prototype.repeat() to simplify code, less code,
more semantically.

PR-URL: #5359
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 21, 2016

Landed in 4d78121

@jasnell jasnell closed this Mar 21, 2016
@JacksonTian JacksonTian deleted the repeat branch March 22, 2016 02:24
@Trott
Copy link
Member

Trott commented Mar 22, 2016

@jasnell Landing this has broken make test because the linter is failing. CI was last run on this more than 4 weeks ago and some rules have been tightened since then. :-(

@Trott Trott mentioned this pull request Mar 22, 2016
Closed
4 tasks
@Trott
Copy link
Member

Trott commented Mar 22, 2016

I'll put together a fix unless someone beats me to it.

@JacksonTian
Copy link
Contributor Author

I am sorry, Let me fix it. @Trott .

@Trott
Copy link
Member

Trott commented Mar 22, 2016

I already opened #5840, but thanks!

@Trott
Copy link
Member

Trott commented Mar 22, 2016

(And no worries, this isn't the first time this sort of thing has happened, the problem is our process not anything you did wrong.)

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

Yep, not your fault at all @JacksonTian ... I'm the one who missed the step. I saw the green CI without checking the date and forgot to run make lint locally before I pushed. It happens, unfortunately.

@Trott
Copy link
Member

Trott commented Mar 22, 2016

Yeah, we've got a lot of opportunities for automation, for sure.

@Fishrock123 Fishrock123 mentioned this pull request Mar 22, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
@JacksonTian @Fishrock123
lib: simplify code with String.prototype.repeat()
7b21c09 
use String.prototype.repeat() to simplify code, less code,
more semantically.

PR-URL: #5359
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

I've manually added the linting changes while backporting. @Trott I've marked your linting PR as don't land on v4

MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@JacksonTian
lib: simplify code with String.prototype.repeat()
af0aba4 
use String.prototype.repeat() to simplify code, less code,
more semantically.

PR-URL: #5359
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 30, 2016
@JacksonTian
lib: simplify code with String.prototype.repeat()
6daebdb 
use String.prototype.repeat() to simplify code, less code,
more semantically.

PR-URL: #5359
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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
benchmark Issues and PRs related to the benchmark subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@JacksonTian @mscdex @targos @jasnell @Trott @MylesBorins