Attempt at partially supporting symbols#34
Conversation
| newlines: "foo\nbar\r\nbaz", | ||
| circular: "[Circular]" | ||
| circular: "[Circular]", | ||
| Symbol(): Symbol(), |
There was a problem hiding this comment.
Ideally
[Symbol()]: Symbol().
| circular: "[Circular]" | ||
| circular: "[Circular]", | ||
| Symbol(): Symbol(), | ||
| Symbol(foo): Symbol(foo), |
There was a problem hiding this comment.
Ideally
[Symbol('foo')]: Symbol('foo')with quotes according to options.
| circular: "[Circular]", | ||
| Symbol(): Symbol(), | ||
| Symbol(foo): Symbol(foo), | ||
| Symbol(foo): Symbol(foo) |
There was a problem hiding this comment.
Ideally
[Symbol.for('foo')]: Symbol.for('foo')with quotes according to options.
| typeof val === 'number' || | ||
| typeof val === 'boolean' || | ||
| typeof val === 'function' || | ||
| typeof val === 'symbol' || |
|
|
||
| if (isObj(val)) { | ||
| const objKeys = Object.keys(val); | ||
| const objKeys = Object.keys(val).concat(Object.getOwnPropertySymbols(val)); |
There was a problem hiding this comment.
I've just reported a bug on MDN’s documentation on Object.keys.
| const eol = objKeys.length - 1 === i ? tokens.newLine : ',' + tokens.newLineOrSpace; | ||
| const key = /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); | ||
| return tokens.indent + key + ': ' + stringify(val[el], opts, pad + opts.indent) + eol; | ||
| const key = typeof el === 'symbol' || /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); |
There was a problem hiding this comment.
Is it correct to bypass the regular expression test when el is a symbol?
| const key = /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); | ||
| return tokens.indent + key + ': ' + stringify(val[el], opts, pad + opts.indent) + eol; | ||
| const key = typeof el === 'symbol' || /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); | ||
| return tokens.indent + String(key) + ': ' + stringify(val[el], opts, pad + opts.indent) + eol; |
There was a problem hiding this comment.
For the executions where key is a symbol. Otherwise 💥.
| newlines: "foo\nbar\r\nbaz", | ||
| [Symbol()]: Symbol(), // eslint-disable-line symbol-description | ||
| [Symbol('foo')]: Symbol('foo'), | ||
| [Symbol.for('foo')]: Symbol.for('foo') |
There was a problem hiding this comment.
Take a look please and compare with the fixture above.
mightyiam
left a comment
There was a problem hiding this comment.
Pareto improvement?
SBoudrias
left a comment
There was a problem hiding this comment.
Code looks pretty good to me. Thanks for sending this out!
| const eol = objKeys.length - 1 === i ? tokens.newLine : ',' + tokens.newLineOrSpace; | ||
| const key = /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); | ||
| return tokens.indent + key + ': ' + stringify(val[el], opts, pad + opts.indent) + eol; | ||
| const key = typeof el === 'symbol' || /^[a-z$_][a-z$_0-9]*$/i.test(el) ? el : stringify(el, opts); |
There was a problem hiding this comment.
Can we make that a if statement. It's getting quite hairy all on a single line.
There was a problem hiding this comment.
@SBoudrias how do you like it now?
See last commit.
|
@SBoudrias please re- 🤓 |
|
Just so I understand this properly, this is stripping any symbol in the object? |
|
@SBoudrias "stripping"? What do you mean by that? On a different topic, as the PR branch is, non-enumerable symbol-key properties are included. I'm guessing we do not wish for that, correct? |
|
Now it excludes non-enumerable symbol-key properties. |
| 'use strict'; | ||
| const isRegexp = require('is-regexp'); | ||
| const isObj = require('is-obj'); | ||
| const getOwnEnumPropSymbols = require('get-own-enumerable-property-symbols'); |
There was a problem hiding this comment.
|
|
||
| if (isObj(val)) { | ||
| const objKeys = Object.keys(val); | ||
| const objKeys = Object.keys(val).concat(getOwnEnumPropSymbols(val)); |
| "json" | ||
| ], | ||
| "dependencies": { | ||
| "get-own-enumerable-property-symbols": "^1.0.1", |
There was a problem hiding this comment.
|
Cool thanks, the latest diff makes more sense to me now. |
1 similar comment
|
Cool thanks, the latest diff makes more sense to me now. |
|
🎆 I think this is semver-major. |
|
I'm going to bump a release tonight. |
No description provided.