Skip to content

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 18, 2017

Add normalizeencoding's test.
normalizeEncoding: https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9
Make use of Arrow Function.

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

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 18, 2017
Copy link
Member

Choose a reason for hiding this comment

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

Optional: This could just be (b) => Buffer.from(b). (The current code base does have a mixture of using {...} and return when it's not required vs. omitting them, so either should be fine, but omitting them is definitely more common.)

Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this arrow function could be (_, j) => j + 0x78 if you like.

@mscdex
Copy link
Contributor

mscdex commented Jan 18, 2017

s/starting/string/ in commit message

@hiroppy hiroppy force-pushed the feature/increase-coverage-of-string-decoder branch from 46a0454 to 7d5c020 Compare January 18, 2017 04:58
@mscdex mscdex added string_decoder Issues and PRs related to the string_decoder subsystem. and removed dont-land-on-v7.x labels Jan 18, 2017
@hiroppy hiroppy force-pushed the feature/increase-coverage-of-string-decoder branch from 7d5c020 to 075cb4b Compare January 18, 2017 04:59
@hiroppy hiroppy changed the title test: increase coverage of starting-decoder test: increase coverage of string-decoder Jan 18, 2017
@hiroppy
Copy link
Member Author

hiroppy commented Jan 18, 2017

Oops... updated. Thanks :)

Copy link
Member

Choose a reason for hiding this comment

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

Nit: does the linter complain if the assignment is removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. I deleted the assignment.

@hiroppy hiroppy force-pushed the feature/increase-coverage-of-string-decoder branch from 075cb4b to bed8892 Compare January 18, 2017 15:43
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding: https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9
@hiroppy hiroppy force-pushed the feature/increase-coverage-of-string-decoder branch from bed8892 to dfebdc6 Compare January 18, 2017 15:45
@lpinca
Copy link
Member

lpinca commented Jan 18, 2017

@addaleax
Copy link
Member

Landed in aebcc1a0d95b5f9e138da3c185c0ba25e5cd0fef

@addaleax addaleax closed this Jan 21, 2017
@hiroppy hiroppy deleted the feature/increase-coverage-of-string-decoder branch January 21, 2017 09:11
@lpinca
Copy link
Member

lpinca commented Jan 21, 2017

@addaleax commit title seems a bit off: "starting-decoder"?

@addaleax
Copy link
Member

@lpinca thanks for catching, force-pushed to fix 😅

addaleax pushed a commit that referenced this pull request Jan 21, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: #10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@hiroppy
Copy link
Member Author

hiroppy commented Jan 21, 2017

Oops, I changed it with another PC. Sorry;(
@addaleax @lpinca Thanks!

targos pushed a commit that referenced this pull request Jan 28, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: #10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: nodejs#10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: nodejs#10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: #10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Landed in v6. This will need a backport PR if it needs to land on v4

MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
Make use of Arrow Function.
Add normalizeencoding's test.
normalizeEncoding:
https://github.com/nodejs/node/blob/master/lib/string_decoder.js#L9

PR-URL: #10863
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

string_decoder Issues and PRs related to the string_decoder subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants