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

customize error message for unrecognized value #149

Merged
merged 1 commit into from
May 30, 2017

Conversation

davidchambers
Copy link
Member

Closes #144

How do these error messages look to you, @panayi?

@JAForbes
Copy link
Member

JAForbes commented May 12, 2017

Listing the entire environment probably isn't helpful for the particular audience this fix is catering for. It will probably overwhelm them (the default env is quite large). I think it would be preferable to simply tell them the likely solution.

Type Constraint Violation: etc etc

sanctuary-def uses an environment to determine if a value belongs to a particular type's domain. 
The value `[Object HTMLElement]` does not have an associated type in the provided environment.
You will probably need to define a type and add it to the environment in order to fix this error.
 
    // There are many ways to define types in sanctuary-def
    // this is the simplest approach, but you may need to use
    // a different type constructor to represent all the possible valid states
    const MyType = $.NullaryType( 

         // This is the name that will appear in the error message
         'YourTypeName'

         // sanctuary types are encouraged to include a documentation url
         , 'your documentation url'

         // this is where you verify a value is a member of your type's domain
         , x => x != null && x.toString() == '[Object HTMLElement]'
    )
    
    // Here we provide our new type to sanctuary-def so it can be aware of our custom values
    const def = $.create({ checkTypes: true, env: $.env.concat([MyType])})
 
For more information about defining custom types, constraints, and the sanctuary-def environment, please visit the documentation: https://....

For a list of existing types defined by the sanctuary community please visit: https://...

Maybe begin with something similar to what we already have, but follow up with something targeted for an absolute beginner someone who doesn't know what polymorphic means, or what an environment is, or how its relevant.

I also agree with @panayi that saying "there is no type" is probably going to confuse them more than "there is no known type". It gives them a hint that they need to give sanctuary-def more information.

Also, maybe sanctuary-def should adopt the first person as Elm does, so

sanctuary-def uses an environment to determine ...

Can become

I use an environment to determine ...

And robotic phrases like

Unrecognized value

Can become

I don't know how to typecheck the value:

Having a persona for the error message can make explanation via prose seem more natural. Its rarely done, but I think its a great idea, that we should steal from Elm, particularly when sanctuary-def is a radically different approach to using JavaScript.

This might be beyond the scope of this PR though, or may not be palatable for other reasons. I'm just imagining how some people I work with would interpret a sanctuary-def error, they would already struggle with the terminology (which I do want them to learn), and the type signatures (which I also want them to learn), but if the error message can guide them towards a solution to the point where they don't need to ask me for help, or open an issue here, or ask in the gitter, it's empowering. And I think that may lead people to have more courage to tackle the intimidating words, and be more likely to define their own types and lean on the library more often.

I don't want us to avoid the real terms, I think that leads to amorphous nonsense, but there's no reason we can't elaborate and define those terms inline.

@JAForbes
Copy link
Member

It might also be helpful to detect common unrecognized values, and provide more specific errors that direct them to a type provided by the community that solves their exact problem.

@davidchambers
Copy link
Member Author

This might be beyond the scope of this PR though, or may not be palatable for other reasons.

I find your suggestions extremely palatable! Rewriting all the existing error messages is out of scope for this pull request, but it is well worth doing. I've opened #150 to track this. Here's what I propose:

  1. We release sanctuary-def@0.11.0. I had planned to merge this pull request before publishing v0.11.0, but it's now clear that it requires more work.

  2. We update the existing error messages to have a friendlier tone and an emphasis on explaining the solution to the problem rather than the cause of the problem.

  3. We update this pull request to use friendlier, more helpful wording.

@JAForbes
Copy link
Member

Good plan! 🎉

@panayi
Copy link

panayi commented May 12, 2017

@davidchambers I like the branching of error-message for differentiating "no known type" Vs "can't satisfy type constraint". I think had I seen this message (not a member of any type in env + a list of existing types), it would be much easier to figure out that a new type def is needed.

Listing the entire environment probably isn't helpful for the particular audience this fix is catering for. It will probably overwhelm them (the default env is quite large)

If the list is too large, it could perhaps print out an array, that the user can expand to inspect.

@JAForbes's suggestion for providing the user ways to resolve the issue in context, sounds awesome.

@davidchambers davidchambers force-pushed the davidchambers/unrecognized branch from 496cab1 to e3c2733 Compare May 22, 2017 05:43
@davidchambers davidchambers force-pushed the davidchambers/unrecognized branch from e3c2733 to 2097059 Compare May 30, 2017 14:36
@davidchambers
Copy link
Member Author

I've changed my mind. This pull request, though imperfect, is an improvement. I'll merge it. We can and should rethink these error messages as part of #150, but that prospect needn't prevent us from making an incremental improvement immediately.

@davidchambers davidchambers merged commit 3137d8b into master May 30, 2017
@davidchambers davidchambers deleted the davidchambers/unrecognized branch May 30, 2017 14:48
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.

3 participants