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

Add test for customDecoder messages and fix typo in messages #666

Merged
merged 3 commits into from Jul 16, 2016

Conversation

Projects
None yet
3 participants
@noahzgordon
Contributor

noahzgordon commented Jul 15, 2016

#639 was closed because @evancz came across the same issue and fixed it in the same way, adding some useful context information. This PR extends the test from #639 to fit his implementation. It also changes the error message to use the function name customDecoder, which was misspelled as @jvoigtlaender points out in elm-lang@7f6e518

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Jul 15, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

process-bot commented Jul 15, 2016

Thanks for the pull request! Make sure it satisfies this checklist. My human colleagues will appreciate it!

Here is what to expect next, and if anyone wants to comment, keep these things in mind.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 15, 2016

Member

Looks good. Can you make the test use String.contains though? I don't think it makes sense to encode the particular wording of the error message in the test, but it does make sense to make sure the users messages appears somewhere in the error message.

Member

evancz commented Jul 15, 2016

Looks good. Can you make the test use String.contains though? I don't think it makes sense to encode the particular wording of the error message in the test, but it does make sense to make sure the users messages appears somewhere in the error message.

@noahzgordon

This comment has been minimized.

Show comment
Hide comment
@noahzgordon

noahzgordon Jul 15, 2016

Contributor

Sounds better. On it.

Contributor

noahzgordon commented Jul 15, 2016

Sounds better. On it.

@noahzgordon

This comment has been minimized.

Show comment
Hide comment
@noahzgordon

noahzgordon Jul 15, 2016

Contributor

Updated with your suggestion.

It gets a bit funky when testing anything but equality for Err values, since to operate on the Err string you have to pattern match and then handle the possibility of Ok values. I've handled this in my projects my using Debug.crash to raise with an informative message on the Ok branch, so that's what I've done here. Let me know if you've found a better solution.

Contributor

noahzgordon commented Jul 15, 2016

Updated with your suggestion.

It gets a bit funky when testing anything but equality for Err values, since to operate on the Err string you have to pattern match and then handle the possibility of Ok values. I've handled this in my projects my using Debug.crash to raise with an informative message on the Ok branch, so that's what I've done here. Let me know if you've found a better solution.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 15, 2016

Member

Probably better to use pass and fail to create the assertion.

Member

evancz commented Jul 15, 2016

Probably better to use pass and fail to create the assertion.

@noahzgordon

This comment has been minimized.

Show comment
Hide comment
@noahzgordon

noahzgordon Jul 15, 2016

Contributor

Cool, didn't know about those functions. Updated to use fail.

Contributor

noahzgordon commented Jul 15, 2016

Cool, didn't know about those functions. Updated to use fail.

@evancz evancz merged commit 2f6c2ae into elm:master Jul 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Jul 16, 2016

Member

Thanks!

Member

evancz commented Jul 16, 2016

Thanks!

@noahzgordon

This comment has been minimized.

Show comment
Hide comment
@noahzgordon

noahzgordon Jul 16, 2016

Contributor

First commits to core! 🎉🎉🎉

Contributor

noahzgordon commented Jul 16, 2016

First commits to core! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment