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

Broken test with runtime error for equality of Values #835

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@ArcherWheeler

ArcherWheeler commented Feb 6, 2017

The added test fails with a runtime exception in the compiled Javascript.

@process-bot

This comment has been minimized.

Show comment
Hide comment
@process-bot

process-bot Feb 6, 2017

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 Feb 6, 2017

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 Mar 7, 2018

Member

I do not think it makes sense to add failing tests in hopes of fixing the underlying issue. It will require some pretty involved design and compiler changes, and in the meantime, we will have to struggle through constantly failing tests. So the timeline is not right to merge this in.

Member

evancz commented Mar 7, 2018

I do not think it makes sense to add failing tests in hopes of fixing the underlying issue. It will require some pretty involved design and compiler changes, and in the meantime, we will have to struggle through constantly failing tests. So the timeline is not right to merge this in.

@evancz evancz closed this Mar 7, 2018

@soenkehahn

This comment has been minimized.

Show comment
Hide comment
@soenkehahn

soenkehahn Mar 7, 2018

@evancz: Sorry, we should have mentioned this, but this PR was never meant to be merged as is, but just as a bug report. We thought it'd be easiest to understand what's going wrong by providing a test-case.

(I've worked with @ArcherWheeler on this.)

soenkehahn commented Mar 7, 2018

@evancz: Sorry, we should have mentioned this, but this PR was never meant to be merged as is, but just as a bug report. We thought it'd be easiest to understand what's going wrong by providing a test-case.

(I've worked with @ArcherWheeler on this.)

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 7, 2018

Member

Equality is "undefined" on JSON values. The fix will probably be disallowing things like functions and JSON values with something like comparable, but there is not a certain design or timeline on anything like that.

It is an issue with a known workaround (do not use equality like that) and a relatively small portion of users will ever run into it at all (maybe <5%?) so it is not top priority at this time. We know about it though, and think it is a design flaw that needs to be fixed! A coherent fix is quite complicated though, so in the same way there is no public issue for "make the parser faster", there is not a public issue for this large project. It's not clear what the path will look like, and the current state of affairs is described in the equality documentation already.

Member

evancz commented Mar 7, 2018

Equality is "undefined" on JSON values. The fix will probably be disallowing things like functions and JSON values with something like comparable, but there is not a certain design or timeline on anything like that.

It is an issue with a known workaround (do not use equality like that) and a relatively small portion of users will ever run into it at all (maybe <5%?) so it is not top priority at this time. We know about it though, and think it is a design flaw that needs to be fixed! A coherent fix is quite complicated though, so in the same way there is no public issue for "make the parser faster", there is not a public issue for this large project. It's not clear what the path will look like, and the current state of affairs is described in the equality documentation already.

@soenkehahn

This comment has been minimized.

Show comment
Hide comment
@soenkehahn

soenkehahn Mar 7, 2018

Interesting, thanks for the explanation. I knew that functions are not comparable, but I'm very surprised that Json.Encode.Value is not, since it's just data. Also for Json.Encode.Value comparisons don't '[...] crash as quickly as possible' (quoted from the documentation you linked to). So for example object [] == object [] doesn't crash. If it would crash it'd be easier to figure out that Json.Encode.Value isn't comparable.

I'm not working in elm currently, so for me it's not urgent to do anything about this, for what it's worth.

soenkehahn commented Mar 7, 2018

Interesting, thanks for the explanation. I knew that functions are not comparable, but I'm very surprised that Json.Encode.Value is not, since it's just data. Also for Json.Encode.Value comparisons don't '[...] crash as quickly as possible' (quoted from the documentation you linked to). So for example object [] == object [] doesn't crash. If it would crash it'd be easier to figure out that Json.Encode.Value isn't comparable.

I'm not working in elm currently, so for me it's not urgent to do anything about this, for what it's worth.

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