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

relax constraints on predicates given to S.get, S.gets, and S.parseJson #424

Merged
merged 1 commit into from
Jul 27, 2017

Conversation

davidchambers
Copy link
Member

Commit message:

These functions facilitate safe interactions with untrusted values. To be maximally useful they should each take a predicate which safely handles any input (even inconsistently typed input).

@@ -3638,15 +3638,21 @@
//.
//. > S.get(S.is(Number), 'x', {})
//. Nothing
//.
Copy link
Member

Choose a reason for hiding this comment

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

The existing example are great. But maybe a little conservative/sensible. I think we can pre-emptively answer some questions about the behaviour of Any in conjunction with the Accessible constraint by demonstrating a completely non sensical input both within and outside an object. Perhaps something like:

S.get( S.is(Number), 'x', { x: 'purple' } ) 
//=> Nothing

S.get( S.is(Number), 'x', 'purple' ) 
// => Type Error
// value 'purple' does not satisfy the Accessible type class.

Copy link
Member Author

Choose a reason for hiding this comment

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

'purple' is meant to satisfy the Accessible constraint but you are right to point out that it does not. It just occurred to me that we might be able to remove the Accessible constraint by applying Object to arguments in a few places. I'll investigate this possibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

'purple' is meant to satisfy the Accessible constraint

Yeah that's what I'm saying. That behaviour is logical to me but could perhaps be illustrated in the examples further.

index.js Outdated
@@ -4037,27 +4043,30 @@
S.parseInt =
def('parseInt', {}, [$.Integer, $.String, $Maybe($.Integer)], parseInt_);

//# parseJson :: (a -> Boolean) -> String -> Maybe b
//# parseJson :: (Any -> Boolean) -> String -> Maybe a
Copy link
Member

Choose a reason for hiding this comment

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

This may be beyond the scope of this PR. But this change could really benefit new users who want to dive in and maybe aren't sure how to bridge the gap from unsafe javascript to sanctuary. Perhaps functions that accept Any and safely return a known type (via Maybe/Either or a default value) could be bundled together in a section: "Migrating from Unsafe Javascript" or "Integrating with an existing Javascript codebase", or something to that affect. It need not be at the top of the file, but it could maybe be linked to quite early on to reassure users that there is an on ramp.

I'm thinking all the parse functions, get, gets, and potentially others.

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 ❤️ this idea!

index.js Outdated
@@ -3621,7 +3621,7 @@
}
S.props = def('props', {a: [Accessible]}, [$.Array($.String), a, b], props);

//# get :: Accessible a => (b -> Boolean) -> String -> a -> Maybe c
//# get :: Accessible a => (Any -> Boolean) -> String -> a -> Maybe b
//.
//. Takes a predicate, a property name, and an object and returns Just the
Copy link
Member

Choose a reason for hiding this comment

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

This might be hard to articulate without it becoming distracting, but it'd be nice to explain why in this instance a predicate is used instead of relying on a type definition.

The reason I bring it up is, I've seen quite a few predicate based schema validation libraries. Very few have their own sophisticated type system like sanctuary-def. It might be confusing / surprising that a sanctuary function is relying on predicates alone.

But I think it does make sense. It doesn't force a user of sanctuary to understand how to define their own types. You can use S.test to make use of types as predicates. And functions like get are helpful when you aren't completely sure what the input will be, e.g. at the edges of a system. There are probably more reasons I'm not thinking of. But I think it's worth while to elaborate on the subject.

These functions facilitate safe interactions with untrusted values.
To be maximally useful they should each take a predicate which safely
handles any input (even inconsistently typed input).
@davidchambers davidchambers force-pushed the davidchambers/relax-predicates branch from 91af0d4 to 7e94210 Compare July 27, 2017 12:46
@davidchambers davidchambers merged commit b7bd054 into master Jul 27, 2017
@davidchambers davidchambers deleted the davidchambers/relax-predicates branch July 27, 2017 12:59
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.

2 participants