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.

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

systematic type checking #50

Merged
merged 1 commit into from
Jun 15, 2015
Merged

systematic type checking #50

merged 1 commit into from
Jun 15, 2015

Conversation

davidchambers
Copy link
Member

Sanctuary currently does type checking in a few places (as those of use who've seen Pattern match failure logged to our console can attest). This pull request addresses two problems with the status quo:

  • when a TypeError is thrown, the error message is rarely helpful; and
  • the site at which a TypeError is thrown is often one or more steps removed from the type mismatch.

See what happens when we provide arguments to a function in the wrong order:

S.at(['foo', 'bar', 'baz'], 1);
// TypeError: at requires a value of type Number as its first argument; received ["foo", "bar", "baz"]

Types are checked as arguments are applied:

S.at('1');
// TypeError: at requires a value of type Number as its first argument; received "1"

This approach was proposed by @paldepind in ramda/ramda#1125 (comment).

It's pleasing that index.js did not grow significantly, despite the fact that we now 🍛 our own functions.

'requires a value of type',
/^function (\w*)/.exec(typePair[1])[1],
'as its',
['first', 'second', 'third', 'fourth', 'fifth'][typePair[0]],

Choose a reason for hiding this comment

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

you really dont like functions that take many arguments ah... what happens if you're out of bound here?

Copy link
Member Author

Choose a reason for hiding this comment

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

This function is only used internally, so we needn't support arities greater than the maximum arity of a Sanctuary function. This is currently three, so we could get away with ['first', 'second', 'third']. If we decide to expose this function at some point we'll need to update it to handle arbitrary arities.

@sharonrapoport
Copy link

well, this code is obviously beyond my understanding, but I very much like the more detailed error logging which I hit all the time :)

@davidchambers
Copy link
Member Author

I very much like the more detailed error logging which I hit all the time

I hope it will be as useful in practice as it is in theory. :)

@benperez
Copy link
Contributor

Looks pretty cool 🐪

@paldepind
Copy link

I hope it will be as useful in practice as it is in theory. :)

I'm sure it will!

var $typePairs = [];
for (var idx = 0; idx < args.length; idx += 1) {
var typePair = typePairs[idx];
if (args[idx] === _) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to wait until Ramda v0.15.0 is available so that I can update this to:

if (args[idx] != null && args[idx]['@@functional/placeholder'] === true) {

See ramda/ramda#1156 for details.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pranavvishnu
Copy link
Contributor

This will change lives.

@michaelckelly
Copy link

This is a really nice change and will make working with Sanctuary a more pleasant experience. 👍

@svozza
Copy link
Member

svozza commented Jun 14, 2015

Very impressive, David. Really looking forward to using this when it is merged.

@davidchambers davidchambers force-pushed the dc-type-checking branch 5 times, most recently from d8da0ed to e33097c Compare June 15, 2015 17:51
case 2: return function(a, b) { return f.apply(this, arguments); };
case 3: return function(a, b, c) { return f.apply(this, arguments); };
}
};
Copy link
Member Author

Choose a reason for hiding this comment

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

R.arity was deprecated in v0.15.0, so we must provide this ourselves. It's not onerous since we don't export any functions with arity greater than three.

davidchambers added a commit that referenced this pull request Jun 15, 2015
@davidchambers davidchambers merged commit 5ffcf85 into master Jun 15, 2015
@davidchambers davidchambers deleted the dc-type-checking branch June 15, 2015 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants