Add prop function #161

svozza · 2016-03-04T22:24:31Z

As discussed in #159. I've a few reservations about this, firstly if I define S.prop as var prop = S.prop = def(... then I get lots of errors in the tests when I replace instances of R.prop in index.js:

TypeError: prop is not a function
at createSanctuary (/home/stefano/git/sanctuary/index.js:9:16296)
at /home/stefano/git/sanctuary/index.js:9:45767
at _ (/home/stefano/git/sanctuary/index.js:9:268)
at Object. (/home/stefano/git/sanctuary/index.js:9:779)

Another thing is that Accessible is a very permissive type for this function, it allows you to pass numbers or booleans in even though they don't support property access so you end up getting error messages that leak implementation details to the user:

S.prop('a', true)
// => TypeError: Cannot use 'in' operator to search for 'a' in true

Also, I think it's strange that a function that operates on Records should consider keys that are present in the prototype chain. What are people's thoughts?

davidchambers · 2016-03-04T22:42:02Z

I think it's strange that a function that operates on Records should consider keys that are present in the prototype chain.

It's not clear to me which is the most appropriate membership check.

I can, though, provide what I believe is a compelling example in favour of key in obj:

//  Pair :: (a, b) -> Pair a b
function Pair(fst, snd) {
  this[0] = fst;
  this[1] = snd;
}

Pair.prototype.length = 2;

Should S.prop('length', new Pair(0, 0)) evaluate to 2? I argue that it should. Otherwise we'd be drawing a distinction between two equivalent implementations (length as a prototype property or as an instance property).

Also, the usual traps with inherited properties affect object-as-dictionary rather than object-as-record. When using an object to keep track of claimed usernames, name in usernames would erroneously evaluate true for name values such as 'toString' and 'valueOf'. When using an object as a record, though, one asks for a known property. Presumably if one writes S.prop('toString', obj) one wants to reference the toString method of obj (regardless of whether it's attached to obj directly).

davidchambers · 2016-03-04T22:43:06Z

index.js

+          return obj[key];
+        } else {
+          throw new TypeError('‘prop’ expected object to have a property named ' +
+          '‘' + key + '’; ' + R.toString(obj) + ' does not');


Let's indent this line to align it with the first ' on the preceding line.

kedashoe · 2016-03-04T23:07:21Z

So kind of a more type-dangerous get? Might be nice to mention get in the description (and this in the description for get.. do we have @sees?), how they're different, when you might use one vs the other.

Should we include props in this PR?

davidchambers · 2016-03-04T23:37:46Z

prop and get are certainly related, but should be used in different contexts. get and gets perform a role similar to that of Haskell's Aeson library: providing a safe way to interact with uncertain data.

We don't use @see (as I detest JSDoc), but we should certainly include reciprocal links and a sentence or two describing the differences.

kedashoe · 2016-03-04T23:50:21Z

prop and get are certainly related, but should be used in different contexts. get and gets perform a role similar to that of Haskell's Aeson library: providing a safe way to interact with uncertain data.

Ya, I was thinking something along those lines.

as I detest JSDoc

ha!

svozza · 2016-03-05T08:35:08Z

You make a convincing case for key in obj, David. Regarding my second point, do you think we should tighten up the definition of Accessible to something like this:

function(x) { return x != null && R.contains(_type(x), ['Array', 'Function', 'Object', 'String']); }

I still have no idea why I can't use S.prop in the file before it's defined even though I declare it as a var (I assumed hoisting would take care of that). It would be nice for use to eat our own dogfood rather than relying on R.prop.

davidchambers · 2016-03-05T19:03:18Z

[D]o you think we should tighten up the definition of Accessible[?]

What's the motivation for doing so?

I still have no idea why I can't use S.prop in the file before it's defined even though I declare it as a var (I assumed hoisting would take care of that).

There are two different concepts in JavaScript referred to as "hoisting": var hoisting, and function declaration hoisting.

When one writes…

(function() {
  console.log(x);
  var x = 42;
}());

var hoisting means one has effectively written…

(function() {
  var x;
  console.log(x);
  x = 42;
}());

This will log undefined rather than throw a ReferenceError as one would expect without var hoisting. Note, though, that it does not log 42.

It would be nice for use to eat our own dogfood rather than relying on R.prop.

We could do so by defining prop near the top of the file…

var prop =
def(…);

then assigning prop to S.prop at the appropriate point in the file.

svozza · 2016-03-06T13:07:43Z

What's the motivation for doing so?

Because R.prop as it is currently defined will not throw the correct type error if we pass a primitive value in and instead we'll get something like TypeError: Cannot use 'in' operator to search for 'a' in true. If Accessible is supposed to represent types that allow property access then why does it allow primitive types that don't?

davidchambers · 2016-03-07T09:50:42Z

Thanks for the explanation, Stefano! I now understand the motivation. :)

If Accessible is supposed to represent types that allow property access then why does it allow primitive types that don't?

Primitive values other than null and undefined support property access:

> true.a
undefined

> 'foo'.toUpperCase()
'FOO'

So Accessible isn't quite right, in this case. We could remove the constraint and determine whether the second argument is valid in the function body. This would enable consistent error messages:

S.prop('x', null);
// ! No ‘x’ property in null

S.prop('x', {});
// ! No ‘x’ property in {}

svozza · 2016-03-07T10:05:51Z

Primitive values other than null and undefined support property access:

Ah yes, through the magic of autoboxing. Although something like 1.toString() is a syntax error so you have to do 1['toString']()

We could remove the constraint and determine whether the second argument is valid in the function body.

Sounds good, I should be able to work on this later on today.

davidchambers · 2016-03-07T10:15:31Z

Although something like 1.toString() is a syntax error so you have to do 1['toString']()

… or (1).toString() or 1..toString(). ;)

svozza · 2016-03-07T10:22:17Z

1..toString()

Ha. That's a new one, how does that work?

davidchambers · 2016-03-07T10:30:48Z

1. is a valid representation of one. The double dot removes the "decimal" ambiguity, I assume.

svozza · 2016-03-10T23:05:12Z

This implementation got really hairy in the end. To support S.prop('length') for strings I've had to manually autobox the final argument; given that I had to override the linter to do so this all seems a bit wrong.

davidchambers · 2016-03-11T00:13:13Z

I don't see a reason to support S.prop('length', 'abc') but not S.prop('toFixed', 42).

We should be able to use the Object function to simplify matters:

-        if (typeof obj === 'string') {
-          /*jshint -W053 */
-          obj = new String(obj);
-          /*jshint +W053 */
-        }
-        if (obj != null && typeof obj !== 'number' &&
-           typeof obj !== 'boolean' && key in obj) {
+        if (obj != null && key in Object(obj)) {

svozza · 2016-03-11T06:40:26Z

Ah yes, that's much better.

⚡

svozza · 2016-03-11T06:46:29Z

Actually, can we remove the null check and reinstate the Accessible type constraint now?

davidchambers · 2016-03-11T07:14:18Z

Actually, can we remove the null check and reinstate the Accessible type constraint now?

Wouldn't that send us back here?

davidchambers · 2016-03-11T07:21:10Z

index.js

+  //# prop :: Accessible a => String -> a -> b
+  //.
+  //. Takes a property name and returns the value of that property from the
+  //. specified object; otherwise throws an error.


How about this?

Takes a property name and a record (an object with known fields of known types), and returns the value of the specified field. If for some reason the record lacks the specified field, a type error is thrown.

For accessing properties of uncertain objects, use get instead.

We should add a "see also" link to the description of get.

svozza · 2016-03-11T07:30:57Z

Wouldn't that send us back here?

Not if we do something like this:

var boxed = Object(obj);
if (key in boxed) {
  return boxed[key];
...

davidchambers · 2016-03-11T07:49:49Z

Ah, I see what you're saying. Sounds great to me!

svozza · 2016-03-11T12:42:42Z

This is shaping up a lot better now. ⚡

davidchambers · 2016-03-11T18:19:07Z

index.js

+  //.
+  //. Takes a property name and an object and returns the value of the
+  //. specified field. If for some reason the record lacks the specified field,
+  //. a type error is thrown.


I suggest two tweaks:

"object with known properties" in place of "object"; and

"property" in place of "field" (since we no longer mention records).

davidchambers · 2016-03-11T18:26:41Z

index.js

+          return boxed[key];
+        } else {
+          throw new TypeError('‘prop’ expected object to have a property ' +
+            'named ‘' + key + '’; ' + R.toString(obj) + ' does not');


When wrapping, I like things to align. Let's use one of the following forms:

throw new TypeError('...' + '...' + '...');

throw new TypeError( '...' + '...' );

davidchambers · 2016-03-11T18:31:57Z

It's a shame we can't leave custom reactions to comments like ⚡

Haha! With :zap:, though, the fact that a comment generates an email notification is usually desirable.

svozza · 2016-03-11T18:34:38Z

Actually, that's a good point.

davidchambers · 2016-03-14T06:56:21Z

test/index.js

+      eq(S.prop('0', [1, 2, 3]), 1);
+      eq(S.prop('length', 'abc'), 3);
+      eq(S.prop('x', Object.create({x: 1, y: 2})), 1);
+    });


Let's include a test for an inherited property. For example:

eq(S.prop('global', /x/g), true);

Although, does Object.create not fulfil that already? x and y live on the created object's prototype.

console.log(Object.create({x: 1, y: 1})); // => {}

The example I suggested is still useful, but it doesn't test what I was hoping to test since global also exists as an instance property:

> Object.prototype.hasOwnProperty.call(/x/g, 'global') true

Although, does Object.create not fulfil that already? x and y live on the created object's prototype.

Good point! I didn't realize that Object.create worked this way. 👍

Yeah it's not obvious, that's why I wrote the test the way I did initially with an object named proto that I then passed into create.

svozza · 2016-03-16T22:41:23Z

Is there any more that needs to be done on this?

davidchambers · 2016-03-16T23:01:38Z

Is there any more that needs to be done on this?

No, but I'd like to do a few other things before merging this pull request:

Update and merge support values of "foreign" types sanctuary-def#38
Publish [email protected]
Submit and merge a pull request to update our sanctuary-def dependency (to address polymorphic functions cannot operate on "foreign" values #165)
Publish [email protected]

I plan to take care of these things this evening. :)

davidchambers · 2016-03-17T07:54:34Z

I plan to take care of these things this evening. :)

Actually, I did my tax return this evening instead. :\

svozza · 2016-03-17T09:21:08Z

Haha. Rather you than me!

davidchambers · 2016-04-13T18:00:27Z

Could you rebase this branch, Stefano?

svozza · 2016-04-13T18:10:52Z

Yes, of course. ⚡

davidchambers · 2016-04-13T18:17:10Z

You'll need to update the expected error messages. ;)

While you're at it, you can take advantage of the assert.throws 👽.

svozza · 2016-04-13T18:17:45Z

Yup, on it!

davidchambers · 2016-04-13T18:47:38Z

test/index.js

+      assert.throws(function() { S.prop('xxx', [1, 2, 3]); },
+                    errorEq(TypeError,
+                            '‘prop’ expected object to have a property ' +
+                            'named ‘xxx’; [1, 2, 3] does not'));


Let's use throws rather than assert.throws for these assertions as well.

Also, now that we're not enforcing a maximum line length let's not wrap the messages. :)

davidchambers · 2016-04-13T19:27:29Z

🌳 Thank you for persevering with this one, Stefano!

svozza · 2016-04-13T20:10:11Z

A surprisingly tricky one!

davidchambers reviewed Mar 4, 2016
View reviewed changes

svozza force-pushed the prop-fn branch from 50e5e59 to 4e913a4 Compare March 5, 2016 08:39

svozza force-pushed the prop-fn branch from 4e913a4 to 08e1bef Compare March 10, 2016 23:02

svozza force-pushed the prop-fn branch from 08e1bef to 2d45952 Compare March 11, 2016 06:39

davidchambers reviewed Mar 11, 2016
View reviewed changes

svozza force-pushed the prop-fn branch from 2d45952 to 86808cf Compare March 11, 2016 12:40

davidchambers reviewed Mar 11, 2016
View reviewed changes

svozza force-pushed the prop-fn branch from 1e32db6 to 4efe4f2 Compare March 11, 2016 18:26

davidchambers reviewed Mar 11, 2016
View reviewed changes

svozza force-pushed the prop-fn branch from 4efe4f2 to 09c23cc Compare March 11, 2016 18:34

davidchambers reviewed Mar 14, 2016
View reviewed changes

svozza force-pushed the prop-fn branch 2 times, most recently from 5622161 to f14847b Compare March 14, 2016 18:47

svozza force-pushed the prop-fn branch from f14847b to 7f0be9c Compare April 13, 2016 18:10

svozza force-pushed the prop-fn branch from 7f0be9c to dab6516 Compare April 13, 2016 18:20

davidchambers reviewed Apr 13, 2016
View reviewed changes

add prop function

c363c0e

svozza force-pushed the prop-fn branch from dab6516 to c363c0e Compare April 13, 2016 19:10

davidchambers merged commit fb92d05 into sanctuary-js:master Apr 13, 2016

svozza deleted the prop-fn branch April 13, 2016 20:10

davidchambers mentioned this pull request May 31, 2016

v0.11.1 release notes #227

Closed

Add prop function #161

Add prop function #161

Conversation

svozza commented Mar 4, 2016

davidchambers commented Mar 4, 2016

Choose a reason for hiding this comment

kedashoe commented Mar 4, 2016

davidchambers commented Mar 4, 2016

kedashoe commented Mar 4, 2016

svozza commented Mar 5, 2016

davidchambers commented Mar 5, 2016

svozza commented Mar 6, 2016

davidchambers commented Mar 7, 2016

svozza commented Mar 7, 2016

davidchambers commented Mar 7, 2016

svozza commented Mar 7, 2016

davidchambers commented Mar 7, 2016

svozza commented Mar 10, 2016

davidchambers commented Mar 11, 2016

svozza commented Mar 11, 2016

svozza commented Mar 11, 2016

davidchambers commented Mar 11, 2016

Choose a reason for hiding this comment

svozza commented Mar 11, 2016

davidchambers commented Mar 11, 2016

svozza commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Mar 11, 2016

svozza commented Mar 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svozza commented Mar 16, 2016

davidchambers commented Mar 16, 2016

davidchambers commented Mar 17, 2016

svozza commented Mar 17, 2016

davidchambers commented Apr 13, 2016

svozza commented Apr 13, 2016

davidchambers commented Apr 13, 2016

svozza commented Apr 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidchambers commented Apr 13, 2016

svozza commented Apr 13, 2016