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 case for #602 pull request fix. #605

Merged
merged 5 commits into from May 17, 2016

Conversation

Projects
None yet
4 participants
@rluiten
Contributor

rluiten commented May 15, 2016

No description provided.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 May 15, 2016

Contributor

@lukewestby @rluiten could one of you please update this PR with context from #602 ?

Contributor

eeue56 commented May 15, 2016

@lukewestby @rluiten could one of you please update this PR with context from #602 ?

@lukewestby

This comment has been minimized.

Show comment
Hide comment
@lukewestby

lukewestby May 15, 2016

Member

@eeue56 sure!

The primary occurrence of this issue is described in this thread from elm-dev. One of the tests in @rluiten's trie package is attempting to decode key-value pairs on something that is not an object. Instead of resulting in Err as expect, it's throwing a runtime error because the err function used on https://github.com/elm-lang/core/blob/master/src/Native/Json.js#L409 does not exist and should be badPrimitive instead. As of Sun May 15 2016 10:58:49 GMT-0500 (CDT) this PR is still waiting on rluiten#1 and will include both the fix and the test case covering this branch of runHelp in Native.Json once merged.

Member

lukewestby commented May 15, 2016

@eeue56 sure!

The primary occurrence of this issue is described in this thread from elm-dev. One of the tests in @rluiten's trie package is attempting to decode key-value pairs on something that is not an object. Instead of resulting in Err as expect, it's throwing a runtime error because the err function used on https://github.com/elm-lang/core/blob/master/src/Native/Json.js#L409 does not exist and should be badPrimitive instead. As of Sun May 15 2016 10:58:49 GMT-0500 (CDT) this PR is still waiting on rluiten#1 and will include both the fix and the test case covering this branch of runHelp in Native.Json once merged.

@eeue56

This comment has been minimized.

Show comment
Hide comment
@eeue56

eeue56 May 17, 2016

Contributor

@evancz ping (this is fairly important)

tl;dr -

Problem

  • dict decoding can cause a runtime error

Solution

  • err should be badPrimitive, this renames that
  • add a test case to make sure dict decoding is more tested
Contributor

eeue56 commented May 17, 2016

@evancz ping (this is fairly important)

tl;dr -

Problem

  • dict decoding can cause a runtime error

Solution

  • err should be badPrimitive, this renames that
  • add a test case to make sure dict decoding is more tested

@evancz evancz merged commit 6c4e9d6 into elm:master May 17, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment