Skip to content

Conversation

bsermons
Copy link
Contributor

@bsermons bsermons commented Jul 8, 2016

show (ErrorAtIndex i e) = "Error at array index " <> show i <> ": " <> show e
show (ErrorAtProperty prop e) = "Error at property " <> show prop <> ": " <> show e
show (JSONError s) = "JSON error: " <> s
show (TypeMismatch exps act) = "Type mismatch: expected " <> to_s exps <> ", found " <> act
where
to_s [] = "???"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to use a non-empty array, for this reason.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but just to say, purescript-nonempty is in contrib currently and this is core.

@paf31
Copy link
Contributor

paf31 commented Jul 8, 2016

Thanks!

@paf31
Copy link
Contributor

paf31 commented Jul 8, 2016

An alternative here is to move the non-empty array to the outer level:

type F = Either (NonEmpty ForeignError)

We probably don't want a dependency on any non core library for NonEmpty though, so we should think carefully about that.

@bsermons
Copy link
Contributor Author

bsermons commented Jul 8, 2016

I can see the value of adding NonEmpty at the top level since it would make it good for use with validating the whole structure but is there a reason it couldn't exist at the TypeMismatch level also?
Seems like it would be hard to get a nice message like the 'expected one of [...], found ...' with it only at the top level.

What are the requirements for a package to be in core vs contrib?

@paf31
Copy link
Contributor

paf31 commented Jul 11, 2016

The Show instance looks good thanks. I think we just need to come to some conclusion regarding NonEmpty.

@bsermons
Copy link
Contributor Author

Yea I just did the easy thing first since I wasn't sure if NonEmpty being in contrib is a show stopper or if there was some other alternative than to using NonEmpty.

I'll continue ahead with it if you don't think it'll be a problem and see how far I can get with the suggestion above with moving the dependency to the outer level.

@bsermons
Copy link
Contributor Author

Played around with using type F = Either (NonEmpty ForeignError). You can see what it would look like here (purescript-foreign) and here (purescript-foreign-generic).

The last commit on each is with TypeMismatch String String and the parent commit is with TypeMismatch (Array String) String.

Some notes:
End up using unsafePartial to go from SigProd's constructors array to NonEmpty
Wasn't sure what to use as the expected type when SigProd's constructors were empty, so just used the name.
Lose the nicer message when using the simple String String constructor.

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