search

avajs / pretty-format

:sparkles: Stringify any JavaScript value
ISC License
1 stars 7 forks source link

Fix how object keys are printed #2

Closed novemberborn closed 7 years ago

novemberborn commented 7 years ago

The original key value was used, rather than the printed name. This caused a crash if the key was a symbol. Regression from a8fc8adfff9ab941a827f639b8522c525e1a5642.

Root cause for https://github.com/avajs/ava/issues/1263.

novemberborn commented 7 years ago

Oops, see #1.

novemberborn commented 7 years ago

Now displays the keys in white, and avoids unnecessarily formatting them as strings.

@vadimdemedes let me know if this is good and I can ship a 1.1.0 to fix the crasher.

caesarsol commented 7 years ago

@novemberborn

👍 !

I think we should get this merged ASAP, since the error message displayed running AVA is quite obscure. (Is there something we could do about that?)

An unrelated question: do you think that non-enumerable Symbol keys should be printed? As of now, I think they are: they were the original cause for my problem and maybe for avajs/ava#1263 too.

novemberborn commented 7 years ago

I think we should get this merged ASAP, since the error message displayed running AVA is quite obscure. (Is there something we could do about that?)

Nah, this is a bug. It's worse in other scenarios: https://github.com/avajs/ava/issues/1263

An unrelated question: do you think that non-enumerable Symbol keys should be printed? As of now, I think they are

I think any key that determines the outcome of t.deepEqual() should be printed. That said:

const foo={}
Object.defineProperty(foo, Symbol.for('foo'), {value:'hi'})

const bar = {}
Object.defineProperty(bar, Symbol.for('foo'),{value:'oh'}

require('lodash.isequal')(foo, bar) // true!

So that would mean no.

caesarsol commented 7 years ago

Nah, this is a bug. It's worse in other scenarios: avajs/ava#1263

Yeah, I was referring to that also!

So that would mean no.

I agree on the same motivation. I could make a PR about this.

novemberborn commented 7 years ago

I could make a PR about this.

Please, feel free! I'm uneasy though with how much we're deviating from pretty-format (which itself has moved into the Jest monorepo). We'll need to address that at some point, and such a PR may be the straw that breaks the camel's back.

caesarsol commented 7 years ago

To fork or not to fork, that is the question...

vadimdemedes commented 7 years ago

Yes, this is good to go! Thanks @novemberborn for fixing it!

vadimdemedes commented 7 years ago

Published @ava/pretty-format@1.1.0.

caesarsol commented 7 years ago

This is officially my first open source PR merged!

(Actually it's only a cherry-picked single-line commit in someone else's PR, but it's a start 😁)

vadimdemedes commented 7 years ago

Oh, @caesarsol, it was yours! Sorry, I assumed it was Mark, because he greated the PR.

vadimdemedes commented 7 years ago

Oh damn, I got which 1-line comment you are referencing. Ok, I gotta go sleep :D

sindresorhus commented 7 years ago

Please, feel free! I'm uneasy though with how much we're deviating from pretty-format (which itself has moved into the Jest monorepo). We'll need to address that at some point, and such a PR may be the straw that breaks the camel's back.

I agree. I don't want us to maintain this forever: https://github.com/avajs/ava/issues/1272