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.

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

feat: Add show derivation #12

Merged
merged 3 commits into from May 15, 2016

Conversation

Projects
None yet
2 participants
@boris-marinov
Contributor

boris-marinov commented May 10, 2016

I took the code from the 'Either' module and put it into a generic derivation.

Added a representation for the constructor functions (All other values have one, so I think it makes sense).

Using getOwnPropertyNames for listing the properties for a given value, because according to section 14.4.2 from this source it returns the values in a deterministic order. According to the MDN docs it doesn't. In practice both getOwnPropertyNames and Object.keys work fine. Not so important since AFAIK names are just for debugging purpose. We may event consider switching to keys to prevent the displaying of non-enumerable properties. What do you think?

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 10, 2016

Member

I guess the representation for the root type is okay. We might want to throw an error if someone gives an empty object to data(), because data('type', {}).derive(show) wouldn't call the derivation function, and it's also most likely a mistake (since it doesn't make much sense to not have any variant).

Member

robotlolita commented May 10, 2016

I guess the representation for the root type is okay. We might want to throw an error if someone gives an empty object to data(), because data('type', {}).derive(show) wouldn't call the derivation function, and it's also most likely a mistake (since it doesn't make much sense to not have any variant).

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 11, 2016

Contributor

We probably want to handle values in a better way, passing primitives to JSON.stringify (so we don't have problems with strings), and just using String(value) otherwise.

How do we deal with plain objects? Meaning how do we know which ones are plain. The ones that don't have a prototype?

The isPlainObject function from lodash is long and weird.

Contributor

boris-marinov commented May 11, 2016

We probably want to handle values in a better way, passing primitives to JSON.stringify (so we don't have problems with strings), and just using String(value) otherwise.

How do we deal with plain objects? Meaning how do we know which ones are plain. The ones that don't have a prototype?

The isPlainObject function from lodash is long and weird.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 11, 2016

Member

I was thinking more along the lines of:

function isPrimitive(x) {
  return typeof x === 'string' 
  ||     typeof x === 'number' 
  ||     typeof x === 'boolean';
}

function showValue(x) {
  return x === null        ?  'null'
  :      x === undefined   ?  'undefined'
  :      Array.isArray(x)  ?  `[${x.map(showValue).join(', ')}]`
  :      isPrimitive(x)    ?  JSON.stringify(x)
  :      /* otherwise */      String(x) // calls .toString()
}

This might give people value: [object Object] when they have a value in the variant that doesn't implement a custom .toString(), but I think at that point they'd want to write a custom .toString() method anyway, since there isn't a simple generic way of describing these objects in general.

Member

robotlolita commented May 11, 2016

I was thinking more along the lines of:

function isPrimitive(x) {
  return typeof x === 'string' 
  ||     typeof x === 'number' 
  ||     typeof x === 'boolean';
}

function showValue(x) {
  return x === null        ?  'null'
  :      x === undefined   ?  'undefined'
  :      Array.isArray(x)  ?  `[${x.map(showValue).join(', ')}]`
  :      isPrimitive(x)    ?  JSON.stringify(x)
  :      /* otherwise */      String(x) // calls .toString()
}

This might give people value: [object Object] when they have a value in the variant that doesn't implement a custom .toString(), but I think at that point they'd want to write a custom .toString() method anyway, since there isn't a simple generic way of describing these objects in general.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 12, 2016

Contributor

I think we still can and should handle the presentation of plain objects in the same way that you handle arrays in your example. Especially since we are overwriting the node.js console representation which handles them by default.

The check whether something is a plain object can be hard, but I think I've come up with a solution - instead of inspecting the object itself we can just check if its toString function is the default one.

I am not sure the referential equality would work in all cases but that can be tested.

Contributor

boris-marinov commented May 12, 2016

I think we still can and should handle the presentation of plain objects in the same way that you handle arrays in your example. Especially since we are overwriting the node.js console representation which handles them by default.

The check whether something is a plain object can be hard, but I think I've come up with a solution - instead of inspecting the object itself we can just check if its toString function is the default one.

I am not sure the referential equality would work in all cases but that can be tested.

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 12, 2016

Member

Oh, I think this is a good direction. 👍

Member

robotlolita commented May 12, 2016

Oh, I think this is a good direction. 👍

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 13, 2016

Contributor

It is.

It does not work across realms though:

win = window.open('https://github.com')
> Window {external: Object, chrome: Object, document: document, speechSynthesis: SpeechSynthesis, caches: CacheStorage…}
win.Object.toString === Object.toString
> false

But I am not even sure if it is worth it to add code that deals with this case. We can do:

const hasCustomToString = (object) => {
  while (Object.getPrototypeOf(object) !== null) {
    if (object.hasOwnProperty('toString')) {
      return true
    }
    object = Object.getPrototypeOf(object)
  }
  return false
}

console.assert(hasCustomToString({}) === false)
console.assert(hasCustomToString({toString(){}}) === true)
console.assert(hasCustomToString(Object.create( { toString(){} }) ) === true)

I don't know...

Contributor

boris-marinov commented May 13, 2016

It is.

It does not work across realms though:

win = window.open('https://github.com')
> Window {external: Object, chrome: Object, document: document, speechSynthesis: SpeechSynthesis, caches: CacheStorage…}
win.Object.toString === Object.toString
> false

But I am not even sure if it is worth it to add code that deals with this case. We can do:

const hasCustomToString = (object) => {
  while (Object.getPrototypeOf(object) !== null) {
    if (object.hasOwnProperty('toString')) {
      return true
    }
    object = Object.getPrototypeOf(object)
  }
  return false
}

console.assert(hasCustomToString({}) === false)
console.assert(hasCustomToString({toString(){}}) === true)
console.assert(hasCustomToString(Object.create( { toString(){} }) ) === true)

I don't know...

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 13, 2016

Contributor

Regarding the throwing of error on invalid input: Yes, we should definitely be doing this for all methods that we provide (unless performance). Maybe folktale validations are a good fit for this problem?

Contributor

boris-marinov commented May 13, 2016

Regarding the throwing of error on invalid input: Yes, we should definitely be doing this for all methods that we provide (unless performance). Maybe folktale validations are a good fit for this problem?

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 13, 2016

Member

Ah, I hadn't thought about the realms stuff. That makes this complicated.

We could use Array.isArray(value) and value.toString() === '[obect Object]' to check for that instead, which works cross-realm. This ignores that people would change the Array's .toString representation, but I think that's an okay compromise.

Member

robotlolita commented May 13, 2016

Ah, I hadn't thought about the realms stuff. That makes this complicated.

We could use Array.isArray(value) and value.toString() === '[obect Object]' to check for that instead, which works cross-realm. This ignores that people would change the Array's .toString representation, but I think that's an okay compromise.

@boris-marinov

This comment has been minimized.

Show comment
Hide comment
@boris-marinov

boris-marinov May 14, 2016

Contributor

Indeed it is.
Fixed.
The funny thing is that the undefined handling was working since JSON.stringify(undefined) returns undefined. Added a check though.

Contributor

boris-marinov commented May 14, 2016

Indeed it is.
Fixed.
The funny thing is that the undefined handling was working since JSON.stringify(undefined) returns undefined. Added a check though.

@robotlolita robotlolita merged commit afecf66 into master May 15, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@robotlolita robotlolita deleted the Show-derivation branch May 15, 2016

@robotlolita

This comment has been minimized.

Show comment
Hide comment
@robotlolita

robotlolita May 15, 2016

Member

Great, thanks :)

The problem with undefined is that its conversion to strings isn't consistent in the standard library, like String(undefined) is "undefined", but [undefined].toString() is "" .-.

Member

robotlolita commented May 15, 2016

Great, thanks :)

The problem with undefined is that its conversion to strings isn't consistent in the standard library, like String(undefined) is "undefined", but [undefined].toString() is "" .-.

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