/core

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

simplified sampleUpdate in Stream.sample #195

Merged
merged 7 commits into from Mar 12, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@jvoigtlaender @evancz
@jvoigtlaender
Contributor

jvoigtlaender commented Mar 12, 2015

... and renamed some type variables so that a and b in SampleEvent and SampleState correspond to the roles of a and b in the type of sample.

jvoigtlaender added some commits Mar 11, 2015

@jvoigtlaender
simplified sampleUpdate in Stream.sample
f7262c0
@jvoigtlaender
further simplification
c6e760d
@jvoigtlaender
Revert "further simplification" 
This reverts commit c6e760d.
72aa47f
@jvoigtlaender
renamed some type variables 
... so that `a` and `b` in `SampleEvent` and `SampleState` correspond to
the roles of `a` and `b` in the type of `sample`.
97a0c31

@jvoigtlaender jvoigtlaender referenced this pull request Mar 12, 2015

Merged

fixing Stream.sample in Elm-0.15 #193

Show outdated Hide outdated src/Stream.elm
{ state = b
, trigger = Just a
sampleUpdate : SampleEvent b a -> SampleState b a -> SampleState b a
sampleUpdate (ms,mu) state =

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

Let's do better names than mu and ms. I don't know what they mean, and saving characters is not a good goal.
Copy URL
@evancz

evancz Mar 12, 2015

Member

Let's do better names than mu and ms. I don't know what they mean, and saving characters is not a good goal.

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

maybeUpdate and maybeSample?
Copy URL
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

maybeUpdate and maybeSample?

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

maybeNewState and trigger seem clearest to me.
Copy URL
@evancz

evancz Mar 12, 2015

Member

maybeNewState and trigger seem clearest to me.
Show outdated Hide outdated src/Stream.elm
in
fold sampleUpdate { state = initialValue, trigger = Nothing } sampleEvents
|> fromVarying
|> filterMap (\state -> Maybe.map (f state.state) state.trigger)
type SampleEvent a b = Sample a | Update b | SampleAndUpdate a b
type alias SampleEvent b a = (Maybe b, Maybe a)

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

What's the reason for switching the order here? Maybe we can give helpful names to these type variables instead?
Copy URL
@evancz

evancz Mar 12, 2015

Member

What's the reason for switching the order here? Maybe we can give helpful names to these type variables instead?

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

Oh, I guess to match the function given to sample?
Copy URL
@evancz

evancz Mar 12, 2015

Member

Oh, I guess to match the function given to sample?

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

Yes, exactly.
Copy URL
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

Yes, exactly.

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

I think this is far enough from convention to be pretty confusing. I'd either change the order of things such that it just matches (switching the args in genericMerge) or not bother with trying to get a and b to match.
Copy URL
@evancz

evancz Mar 12, 2015

Member

I think this is far enough from convention to be pretty confusing. I'd either change the order of things such that it just matches (switching the args in genericMerge) or not bother with trying to get a and b to match.

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

I also think the type alias is not making things clearer at this point.
Copy URL
@evancz

evancz Mar 12, 2015

Member

I also think the type alias is not making things clearer at this point.
@jvoigtlaender

This comment has been minimized.

Show comment
Hide comment
@jvoigtlaender

jvoigtlaender Mar 12, 2015

Contributor

How about now? The record type alias with named components makes it clearer?
Contributor

jvoigtlaender commented Mar 12, 2015

How about now? The record type alias with named components makes it clearer?

@evancz evancz merged commit e42a973 into elm:master Mar 12, 2015
1 check failed

1 check failed

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

This comment has been minimized.

Show comment
Hide comment
@evancz

evancz Mar 12, 2015

Member

I did some last bits of cleanup, but I this is overall a lot nicer, thank you!
Member

evancz commented Mar 12, 2015

I did some last bits of cleanup, but I this is overall a lot nicer, thank you!

@jvoigtlaender jvoigtlaender deleted the jvoigtlaender:alternativeSample branch Mar 12, 2015

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