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

lib,test,benchmark: simplify code with String.prototype.repeat() by JacksonTian · Pull Request #5359 · nodejs/node · GitHub

/ node Public

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

Closed
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:repeat
ConversationCommits (1)ChecksFiles changed
Closed

lib,test,benchmark: simplify code with String.prototype.repeat()#5359
JacksonTian wants to merge 1 commit into
nodejs:masterfrom
JacksonTian:repeat

Conversation

JacksonTian commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

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

JacksonTian commented Feb 22, 2016

Copy link
Copy Markdown
Contributor Author

like #5311 .

mscdex commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

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

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 commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

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 commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

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 force-pushed the repeat branch 2 times, most recently from 00753aa to 34c6b03 Compare February 22, 2016 09:09

JacksonTian commented Feb 22, 2016

Copy link
Copy Markdown
Contributor Author

Hi @mscdex , I updated the PR.

mscdex commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

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

mscdex commented Feb 22, 2016

Copy link
Copy Markdown
Contributor

CI is all green

targos commented Feb 22, 2016

Copy link
Copy Markdown
Member

LGTM

lib,test,benchmark: simplify code with String.prototype.repeat()
08130dd 
use String.prototype.repeat() to simplify code, less code, more
semantically.

JacksonTian commented Mar 2, 2016

Copy link
Copy Markdown
Contributor Author

Rebased with master.

jasnell commented Mar 2, 2016

Copy link
Copy Markdown
Member

LGTM

ronkorving mentioned this pull request Mar 7, 2016
Closed
2 tasks
jasnell pushed a commit that referenced this pull request Mar 21, 2016
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 commented Mar 21, 2016

Copy link
Copy Markdown
Member

Landed in 4d78121

jasnell closed this Mar 21, 2016
JacksonTian deleted the repeat branch March 22, 2016 02:24

Trott commented Mar 22, 2016

Copy link
Copy Markdown
Member

@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 mentioned this pull request Mar 22, 2016
Closed
4 tasks

Trott commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

JacksonTian commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

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

Trott commented Mar 22, 2016

Copy link
Copy Markdown
Member

I already opened #5840, but thanks!

Trott commented Mar 22, 2016

Copy link
Copy Markdown
Member

(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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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 commented Mar 22, 2016

Copy link
Copy Markdown
Member

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

Fishrock123 mentioned this pull request Mar 22, 2016
Merged
Fishrock123 pushed a commit that referenced this pull request Mar 22, 2016
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 commented Mar 30, 2016

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

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

Back | FazBrowse Home | New Git URL