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

fixing Stream.sample in Elm-0.15 #193

Merged
merged 1 commit into from Mar 11, 2015

Conversation

Projects
None yet
2 participants
@jvoigtlaender
Contributor

jvoigtlaender commented Mar 10, 2015

The current 0.15 version is broken for situations like this:

main = Varying.map Graphics.Element.show (Varying.fromStream (0,0) s)

s = let v = Mouse.position
    in Stream.sample always v (Stream.fromVarying v)

The output is never updating! That cannot be what is supposed to happen, right?

The commit here fixes this in the way that changes the existing code of Stream.sample the least.

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 11, 2015

Member

Nice find, thank you for the fix!

I wonder if this pattern of A, B, or both will show up in other places.

Member

evancz commented Mar 11, 2015

Nice find, thank you for the fix!

I wonder if this pattern of A, B, or both will show up in other places.

evancz pushed a commit that referenced this pull request Mar 11, 2015

Merge pull request #193 from jvoigtlaender/fixSample
fixing Stream.sample in Elm-0.15

@evancz evancz merged commit 867c9d7 into elm:master Mar 11, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@jvoigtlaender jvoigtlaender deleted the jvoigtlaender:fixSample branch Mar 11, 2015

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 11, 2015

Contributor

In such situtations, what I'd actually do is to model the thing as (Maybe A, Maybe B) rather than via type SomeOfAB a b = OnlyA a | OnlyB b | Both a b. For the concrete case, doing it like this: jvoigtlaender@f7262c0. There, it simplifies the definition of the sampleUpdate function quite a bit.

The only reason I didn't right away do this in the pull request here is that I wanted it to be the minimal fix (in terms of changed lines and cognitive load).

Contributor

jvoigtlaender commented Mar 11, 2015

In such situtations, what I'd actually do is to model the thing as (Maybe A, Maybe B) rather than via type SomeOfAB a b = OnlyA a | OnlyB b | Both a b. For the concrete case, doing it like this: jvoigtlaender@f7262c0. There, it simplifies the definition of the sampleUpdate function quite a bit.

The only reason I didn't right away do this in the pull request here is that I wanted it to be the minimal fix (in terms of changed lines and cognitive load).

@evancz

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

That looks great to me, do you mind making a PR for that?

I also changed fps to a Stream which I think simplified stuff there, but I may have messed things up.

Member

evancz commented Mar 12, 2015

That looks great to me, do you mind making a PR for that?

I also changed fps to a Stream which I think simplified stuff there, but I may have messed things up.

@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

Have added a PR here: https://github.com/elm-lang/core/pull/195.

I'll also try to take a look at the new version of fps separately.

Contributor

jvoigtlaender commented Mar 12, 2015

Have added a PR here: https://github.com/elm-lang/core/pull/195.

I'll also try to take a look at the new version of fps separately.

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