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

util: fix inspecting symbol key in string #11672

Closed
wants to merge 3 commits into from
Closed

util: fix inspecting symbol key in string #11672

wants to merge 3 commits into from

Conversation

barinali
Copy link
Contributor

@barinali barinali commented Mar 3, 2017

I had to use type check to avoid checking Symbols in numeric conditions.

Fixes: #11659

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)
  • util

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Mar 3, 2017
cjihrig
cjihrig requested changes Mar 3, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

Please provide a test.

@barinali
Copy link
Contributor Author

barinali commented Mar 3, 2017

@cjihrig sorry for that. I've added a test case right now. I hope it would be enough.

@addaleax addaleax mentioned this pull request Mar 6, 2017
Closed
@lpinca
Copy link
Member

lpinca commented Mar 6, 2017 •
edited
Loading

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

@lpinca
Copy link
Member

lpinca commented Mar 6, 2017

@barinali can you please run make lint and fix the linting issues?

@barinali
Copy link
Contributor Author

barinali commented Mar 6, 2017

@lpinca I'm on it right now.

@barinali
Copy link
Contributor Author

barinali commented Mar 6, 2017

@lpinca I've fixed issues.

@lpinca
Copy link
Member

lpinca commented Mar 6, 2017

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

@lpinca
Copy link
Member

lpinca commented Mar 6, 2017

Landed in d0b93c9.

@lpinca lpinca closed this Mar 6, 2017
lpinca pushed a commit that referenced this pull request Mar 6, 2017
@barinali @lpinca
util: fix inspecting symbol key in string
d0b93c9 
PR-URL: #11672
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 7, 2017
@barinali @evanlucas
util: fix inspecting symbol key in string
7b84363 
PR-URL: #11672
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
Merged
evanlucas added a commit that referenced this pull request Mar 8, 2017
@evanlucas
2017-03-08, Version 7.7.2 (Current)
a7d3798 
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) #11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) #7360
* util: fix inspecting symbol key in string (Ali BARIN) #11672

PR-URL: #11745
@barinali barinali deleted the bugfix/issue-11659 branch March 8, 2017 14:01
evanlucas added a commit that referenced this pull request Mar 8, 2017
@evanlucas
2017-03-08, Version 7.7.2 (Current)
b394cf3 
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) #11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) #7360
* util: fix inspecting symbol key in string (Ali BARIN) #11672

PR-URL: #11745
@italoacasas italoacasas mentioned this pull request Mar 14, 2017
Merged
imyller added a commit to imyller/meta-nodejs that referenced this pull request Mar 16, 2017
@imyller
2017-03-08, Version 7.7.2 (Current)
ca1cc7d 
    Notable changes:

    * doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) nodejs/node#11676
    * tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) nodejs/node#7360
    * util: fix inspecting symbol key in string (Ali BARIN) nodejs/node#11672

    PR-URL: nodejs/node#11745

Signed-off-by: Ilkka Myller <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@evanlucas @jungx098
2017-03-08, Version 7.7.2 (Current)
1444cfb 
Notable changes:

* doc: add `Daijiro Wachi` to collaborators (Daijiro Wachi) nodejs#11676
* tty: add ref() so process.stdin.ref() etc. work (Ben Schmidt) nodejs#7360
* util: fix inspecting symbol key in string (Ali BARIN) nodejs#11672

PR-URL: nodejs#11745
@MylesBorins
Copy link
Contributor

The test fails when cherry-picked to v6.x

Path: parallel/test-util-inspect
util.js:395
      return !(key >= 0 && key < raw.length);
               ^

TypeError: Cannot convert a Symbol value to a number
    at util.js:395:16
    at Array.filter (native)
    at formatValue (util.js:394:17)
    at Object.inspect (util.js:183:10)
    at Object.<anonymous> (/Users/mborins/code/node/v6.x/test/parallel/test-util-inspect.js:49:25)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)

Please backport if it is applicable

barinali added a commit to barinali/node that referenced this pull request May 7, 2017
@barinali
util: fix inspecting symbol key in string
5422cf5 
PR-URL: nodejs#11672
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@barinali
Copy link
Contributor Author

barinali commented May 7, 2017

@MylesBorins tests (make -j4 test) fails on v6.x and v6.x-staging even without this commit. But when I run this test standalone, ./node ./test/parallel/test-util-inspect, it passes. Should I create a pull request?

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

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot convert a Symbol value to a number at util.js:398:16
6 participants
@barinali @lpinca @MylesBorins @jasnell @cjihrig @nodejs-github-bot